0

I have the following program:

#include <stdio.h>

#define MAXLEN 100

char *my_strcat(char *strp1,char *strp2) {

    char str[MAXLEN], *strp;
    strp = str;
    while (*strp1 != '\0') {
        *strp++ = *strp1++;
    }
    while (*strp2 != '\0') {
        *strp++ = *strp2++;
    }
    *strp = '\0';
    strp = str;
    return strp;
}

void test_strcat(void) {

char *strp1, *strp2, *strp3, str1[MAXLEN], str2[MAXLEN];
printf("Testing strcat! Give two strings:\n");
gets_s(str1, sizeof(str1));
gets_s(str2, sizeof(str2));
strp1 = str1;
strp2 = str2;
strp3 = my_strcat(strp1, strp2);
printf("Concatenated string: %s", strp3);

}


int main(void) {
test_strcat();
}

The function char *mystrcat is supposed to concatenate two strings, and I test it with test_strcat. The program runs without errors but instead of printing the concatenated string a smiley symbol is printed. I have gone through the program with debugging and it appears that the result sent back by my_strcat is the correct string. However, when going into the last line where strp3 is supposed to be printed it appears red in the debugging tool, implying that its value is about to change. After the printf call, strp3 no longer points to the concatenated string. Anyone knows what could be causing this error?

  • You are returning a local variable. You need to use `malloc` instead of the array. `char *strp = new char[MAXLEN];` – 001 Jun 13 '14 at 12:22
  • What is `str[MAXLEN]`? It appears to be uninitialized, never used, but it is what you return from your function... `strp = str; return strp;` – wolfPack88 Jun 13 '14 at 12:23
  • Try by `char *strp=(char *)malloc(MAXLEN*sizeof(char));` in `*my_strcat()` function. – Jayesh Bhoi Jun 13 '14 at 12:24
  • possible duplicate of [Why should a function not return a local array?](http://stackoverflow.com/questions/16680092/why-should-a-function-not-return-a-local-array) – n. m. could be an AI Jun 13 '14 at 12:27

2 Answers2

1

Here is the problem:

char str[MAXLEN], *strp;
strp = str;  // str is a local variable
...
return strp; // <<== WRONG!!!

Since str is a local variable that disappears as soon as you return, the value pointed to by strp becomes invalid the instance the caller gets the control back.

Use malloc instead of allocating memory in the automatic storage area (i.e. on the stack) will fix this problem:

char *str = malloc(strlen(strp1)+strlen(strp2)+1);
char *strp = str;
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Thanks a bunch! I realized it would also be possible to add a third argument to the function my_strcat that is a pointer to the memory where the new string is supposed to be stored. Which of these possibilities would be preferable in general? – user3337301 Jun 13 '14 at 12:53
  • @user3337301 In general, both these approaches are valid. The `malloc` approach is valid when you do not care where the string is stored, and you do not mind spending a few extra nanoseconds managing memory. An approach that takes an output buffer is inherently less safe, because it opens up possibilities for buffer overruns, but at the same time it lets you deal with stack and/or static memory, potentially saving on CPU time where it counts (this becomes relevant when you write device drivers or similar low-level code). – Sergey Kalinichenko Jun 13 '14 at 13:10
0

I suggest you 2 ways as following.

first,

char *my_strcat(char *strp1,char *strp2) {
    static char str[MAXLEN * 2]; /* from char str[MAXLEN] */

second,

char *my_strcat(char *strp1,char *strp2) {
    char *str = malloc(strlen(strp1) + strlen(strp2) + 1);

because in my_strcat function, you allocated the str as auto variable. When my_strcat function is finish, str will be freed.

xiaodongjie
  • 246
  • 2
  • 5