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;
    }

Answer 1

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.

Answer 2

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;

Answer 3

There are pretty much errors in your code:

1. 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.
2. 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)
3. 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).

Answer 4

Some observations:

1. You're not copying the termination, leaving a non-terminated string in dst. This is the cause of the actual problems.
2. 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.
3. 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.
4. You can't have a function beginning with str, that "namespace" is reserved for the standard library.

Answer 5

Try this:

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

It may be helpful.

Answer 6

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';

Answer 7

The idiomatic way is

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