Implementation of string concatenation

Question

This is the strcat function that I have implemented, but I get a segmentation fault when I go to the line *dst++ = *src++;. I have incremented src till '\0' as I want to append the next string starting from there. Can you please tell me why it gives segmentation fault? Am I doing any logical mistake by doing *dst++ = *src++;?

char *strconcat(char *dst, char *src)
{
    char *fdst;
    fdst = dst;
    if (dst == '\0' || src == '\0')
    return fdst;

    while (*dst != '\0')
        dst++;
    while (*src != '\0')
        *dst++ = *src++;

    return fdst;
}

Hey i went through many solution given below and i made following changes but still i get segmentation problem when i start concatenate two strings, here is my entire code

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char *strconcat(char *dst, const char *src);
void main()
{
    char *str1 = "Stack";
    char *str2 = "overflow";
    printf("MY CONCAT:%s\n",strconcat(str1, str2));
    printf("INBUILT CONCAT:%s\n",strcat(str1, str2));
}

char *strconcat(char *dst, const char *src)
{
    char *fdst;
    int dst_len = 0, src_len = 0;

    dst_len = strlen(dst);
    src_len = strlen(src);
    fdst = (char *) malloc(dst_len + src_len + 1);

    fdst = dst;
    if (src == NULL)
        return fdst;

    while(*dst)
    {
       dst++;
       fdst++;
    }
    while(*src)
       *fdst++ = *src++;
       *fdst = '\0';

    return fdst;
    }

You've forgotten to null-terminate the destination string. But that shouldn't crash this code. — Jonathan Leffler
– Jonathan Leffler, Commented Nov 20, 2012 at 8:42
You probably don't have dst large enough to contain both dst and src. — Vyktor
– Vyktor, Commented Nov 20, 2012 at 8:43
make sure that destination is large enough to contain the source and destination. adding to Vyktor, make sure you put \0 at the end of destination too. — Naga Sandeep
– Naga Sandeep, Commented Nov 20, 2012 at 8:49
I have to agree with @hmjd; please show us how you're invoking this code. The only significant problem is that you do not null terminate the output string. That could easily lead to problems if you're testing the code in a loop, concatenating each time with the previous string. — Jonathan Leffler
– Jonathan Leffler, Commented Nov 20, 2012 at 8:59

thebjorn · Accepted Answer · 2012-11-20 08:41:46Z

2

The idiomatic way is

while (*dst++ = *src++);

answered Nov 20, 2012 at 8:41

thebjorn

27.6k12 gold badges107 silver badges152 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Jonathan Leffler Over a year ago

That will generate a warning from the GCC under -Wall. Better to write it as while ((*dst++ = *src++) != '\0');, preferably with the semi-colon on the next line. But this does ensure null termination, unlike the original code.

Naga Sandeep Over a year ago

*dst should also have '\0' as the termination character.

Jonathan Leffler Over a year ago

@mi32labs: the target string is null terminated with this loop, because the assigment completes before the character is compared to zero.

thebjorn Over a year ago

The nice thing about idiomatic usages is that everyone immediately knows what is going on: while (*dst++ = *src++); ... aha, it's a strcpy. More importantly, you'll know (from recognition) that it is "doing it right" (i.e. it copies the null terminator).

SomeWittyUsername Over a year ago

This is copy, not concatenation!

|

unwind · Accepted Answer · 2012-11-20 09:06:57Z

2

Some observations:

You're not copying the termination, leaving a non-terminated string in dst. This is the cause of the actual problems.
This: if(dst == '\0'||src == '\0') is weird, if the intent was comparing against NULL you should do so more directly and not use character literals.
The src argument should be const char * since it's read-only. Using const for pointers that are "input" arguments is a very good idea, since it communicates intent right there in the prototype. It also helps avoid mistakingly writing to the wrong string.
You can't have a function beginning with str, that "namespace" is reserved for the standard library.

edited Nov 20, 2012 at 9:06

answered Nov 20, 2012 at 8:42

unwind

402k64 gold badges492 silver badges620 bronze badges

3 Comments

Jonathan Leffler Over a year ago

'\0' is a funny way of writing a null pointer constant, but it is a null pointer constant.

unwind Over a year ago

@JonathanLeffler True, I mis-interpreted the intent, edited above. I'd go so far as to call it a really cr*ppy way to write a pointer constant, actually. Thanks, though. :)

Jonathan Leffler Over a year ago

There are a few worse ways of writing a null pointer constant, but not many of them.

nguns · Accepted Answer · 2012-11-21 06:01:41Z

Multiple problems here:

PROBLEM - I

The memory allocated with malloc

    fdst = (char *) malloc(dst_len + src_len + 1);

is lost as some lines later you are doing this:

    fdst = dst;

replacing the address returned by malloc in 'fdst' with the address of the target string..!! Hope you fix it on your own, its darn simple.

PROBLEM - II

After fixing that above problem, you'll have to fix this:

while(*dst)
{
   dst++;
   fdst++;
}

don't only increment, you'll also have to copy your characters from dst to fdst as this will be your concatenated string.

PROBLEM - III

finally, you are doing this in the end..!!

return fdst;

you realize the mistake there right? Hope you can take care of that [ Hint: save the starting address and return it in the end, not the incremented pointer ;) ]

Note: Not an optimised solution but, fixes your code.

MOHAMED · Accepted Answer · 2012-11-20 09:03:51Z

1

Possible crash causes:

1) may be the length of your dst is not enougth to support concatunation of src and dst.

2) may be you have called your function with input char pointers which they are not pointed to allocated memory (staic or dynamic)

3) may be your input dst char pointer is pointing to a constant string.

another remark you have to finish your dst string with '\0' after the second while

 while (*src != '\0')
        *dst++ = *src++;
 *dst='\0';

edited Nov 20, 2012 at 9:03

answered Nov 20, 2012 at 8:57

MOHAMED

44k62 gold badges178 silver badges293 bronze badges

Comments

Heena Hussain · Accepted Answer · 2012-11-20 09:06:31Z

1

Try this:

while(*original)
  original++;
while(*add)
{
  *original = *add;
  add++;
  original++;
}
*original = '\0';

It may be helpful.

answered Nov 20, 2012 at 9:06

Heena Hussain

3,7011 gold badge22 silver badges21 bronze badges

Comments

codewarrior · Accepted Answer · 2012-11-20 09:17:22Z

There are pretty much errors in your code:

if (dst == '\0' || src == '\0') what you are trying by checking this. First of all this is not clear condition - you should use if (dst == NULL || src == NULL) or if (*dst == '\0' || *src == '\0') to be more accurate and make this more clear. Second even if this condition would be right (if (*dst == '\0' || *src == '\0')) you are not achieving what concatenation should. At least if *src == '\0' you should probably return original string - dst.
You should probably check if dst is long enough to store your new string or you should allocate new buffer inside function big enough to hold both dst and src (malloc (strlen(dst) + strlen(src) + 1) - note extra +1 for holding terminating '/0' character)
You are not terminating your result string.

And the answer for your questions: segmentation fault is probably because your dst is NOT long enough to hold both src and dst. You can use hint in point 2. to modify your code or you can declare bigger buffer outside function that will have size at least (strlen(dst) + strlen(src) + 1. Another reason could be calling this function with constant string e.g char *str = "string";. In this case most probably string is constant and you are not allowed to modify it (in most operating system this will be located in non-modifiable part of program and you will have only pointer to this location).

hack3r · Accepted Answer · 2012-11-20 10:44:26Z

Your code is correct , see below for explanation !

I think you are using char * for the src and dst string in the caller.

Using an array declaration there will help, since your program is crashing at

*dst++ = *src++;

because dst and src point to strings which are constants and cannot be modified.

In the following code I have just added main, and your function is unchanged !

#include<stdio.h>

char *strconcat(char *dst, char *src)
{
    char *fdst;
    fdst = dst;
    if (dst == '\0' || src == '\0')
        return fdst;

    while (*dst != '\0')
        dst++;
    while (*src != '\0')
        *dst++ = *src++;

    return fdst;
}

void main()
{
 char dest[10] = "one" ;
 char src[10] = "two" ;

printf("\n%s " , strconcat( dest , src ) ) ;
}

Although you need to change the if statement to

 if (*dst == '\0' || *src == '\0')
            return fdst;

Collectives™ on Stack Overflow

Implementation of string concatenation

7 Answers 7

6 Comments

3 Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

6 Comments

3 Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related