How to fix this function so that it returns concatenation of two strings?

Question

I am trying to write a custom function in C that will concatenate two strings. So far, I have come up with:

char *strcat406(char *str1, char *str2) {

    int str1length = 0;
    int str2length = 0;
    char newStr[str1length + str2length];
    int i;

    for (i = 0; i < str1[i] != '\0'; i++)
        str1length += 1;

    for (i = 0; i < str2[i] != '\0'; i++)
        str2length += 1;

    for (i = 0; i < str1[i] != '\0'; i++)
        newStr[i] = str1[i];

    for (i = i; i < str2[i] != '\0'; i++)
        newStr[i] = str2[i];

    return newStr;

}

I believe the code should work except for the line that reads return newStr;, Xcode is giving me an error that reads "address of stack memory associated with local variable (x) returned" and I think that is why I am not getting the string array printed in main like I want it to.

From my research, it seems like this is happening because the memory for the array returned is freed, and so I get a result I do not want, though I have not found a single answer on this site or even in the C documentation that has worked for me.

How can I change my function so that it returns the concatenation of two strings?

`though I have not found a single answer on this site or even in the C manual that has worked for me.`...tell me why I don't believe that... — Sourav Ghosh, Apr 15 '16 at 15:27

score 2 · Answer 1 · answered Apr 15 '16 at 15:29

Two things to tell

Do not rely on VLAs, as they have been made as optional part as of C11 standard.
VLAs need not be allocated in heap memory at least the standard does not mandate anything.

Simple solution: ^{(I did not check the concatenation logic involved)}

Make newStr a pointer and use dynamic memory allocation[ malloc()/ calloc()].

Dynamically allocated memory remains in scope until deallocated programatically, so they are no limited to the function scope in which they are defined. In the caller function, you need to free up the memory after you are done with it.

Paul Ogilvie · Accepted Answer · 2016-04-15T15:41:01.757

You must calculate the length of the resulting string, then allocate the memory for it, then copy it, then return it. You are now using a VLA. It is better to use malloc:

char *strcat406(char *str1, char *str2) {

    int str1length = 0;
    int str2length = 0;
    char *newStr;  // note: length not known yet
    int i, j;

    for (i = 0; i < str1[i] != '\0'; i++)
        str1length += 1;

    for (i = 0; i < str2[i] != '\0'; i++)
        str2length += 1;

    newstr= malloc (str1length + str2length + 1);

    for (i = 0; str1[i] != '\0'; i++)
        newStr[i] = str1[i];

    for (j = 0 ;str2[j] != '\0'; j++)
        newStr[i+j] = str2[j];

    newstr[i+j]= '\0';
    return newStr;
}

There ara better ways to do this, but I stick to your approach. Don't forget to call free for the new string when you no longer need it.

EDIT The better approach being:

char *strcat406(char *str1, char *str2)
{
    char *newStr= malloc(strlen(str1)+strlen(str2)+1);
    strcpy(newStr,str1);
    strcat(newStr,str2);
    return newStr;
}

Why not instead of the loops just call `strcpy(...); strcat(...)`? — alk, Apr 15 '16 at 15:35
@alk, as I say: "There are better ways to do this, but I stick to your approach". That is so he learns what was wrong with his initial approach. — Paul Ogilvie, Apr 15 '16 at 15:36

How to fix this function so that it returns concatenation of two strings?

2 Answers2