String concatenate error with malloc dynamic memory allocation

Question

String *char concatenate error with malloc dynamic memory allocation

I want to make a function to concatenate strings, it works but it gives an error and the processor restarts, I think there's something wrong with pointers, but I do not know what it is, a problem of memory allocation.

Thanks in advance!

char *buf;

int main(void) {
    // ...

    WriteString("#INIT.\r\n"); //serial output

    buf = "";

    while(1)
    {
        char *str1 = "qwe";
        char *str2 = "asd";
        char *str3 = "zxc";
        char *str4 = "123";

        buf = my_strcat(buf,str1);
        buf = my_strcat(buf,str2);
        buf = my_strcat(buf,str3);
        buf = my_strcat(buf,str4);

        WriteString(buf); //serial output

        free(buf);
    }
}

char *my_strcat(const char *str1, const char *str2) {
    char *new_str;
    new_str = malloc(strlen(str1)+strlen(str2)+1);
    new_str[0] = '\0';
    strcat(new_str,str1);
    strcat(new_str,str2);
    return new_str;
}

Serial output...

#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
#INIT.
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123
qweasdzxc123

Iharob Al Asimi · Accepted Answer · 2015-01-08T19:04:22.557

4

Your first call to my_strcat has undefined behavior because buf was not initialized before. Precisely this line is the problem

new_str = malloc(strlen(str1)+strlen(str2)+1);

strlen(str1) where str1 is uninitialized.

Suggestion, use realloc

char *my_strcat(char *str1, const char *str2)
{
    char  *new_str;
    size_t length;
    size_t str1length;

    if (str2 == NULL)
        return str1;
    str1length = 0;
    if (str1 != NULL)
        str1length = strlen(str1);
    length  = strlen(str2) + str1length;
    new_str = realloc(str1, 1 + length);
    if (new_str == NULL)
        return str1;
    new_str[str1length] = '\0';

    strcat(new_str, str2);

    return new_str;
}

and

char *buf;
char *str1 = "qwe";
char *str2 = "asd";
char *str3 = "zxc";
char *str4 = "123";

buf = NULL;
buf = my_strcat(buf, str1);
buf = my_strcat(buf, str2);
buf = my_strcat(buf, str3);
buf = my_strcat(buf, str4);

edited Jan 08 '15 at 19:04

answered Jan 08 '15 at 18:55

Iharob Al Asimi

52,653
6
59
97

move this line `buf = "";` inside the loop then, since in subsequent calls you are use the `free`'d `buf`, but still your solution will leak memory as the **R Sahu**'s answer points out, you should try my `realloc` version. – Iharob Al Asimi Jan 08 '15 at 19:08
iharob, *buf need to be global, to be accessed by others methods, like a print buffer. – Marco Costa Jan 25 '15 at 15:25
That is not a good reason, make each function that needs to access `buf` take a `char` pointer parameter and pass `buf` to it, a functions that works on a global variable doesn't make much sense, you cannot reuse it's code for example. Avoid global variables as much as possible. – Iharob Al Asimi Jan 25 '15 at 15:28

score 2 · Answer 2 · answered Jan 08 '15 at 19:05

You have memory leaks in the while loop and are running out of memory.

    buf = my_strcat(buf,str1); // Got some new memory
    buf = my_strcat(buf,str2); // Got some more new memory without freeing the previous memory
                               // The previous memory is lost. You don't even have a pointer 
                               // to it any more.

    buf = my_strcat(buf,str3); // Ditto
    buf = my_strcat(buf,str4); // Ditto

What you need:

    char* temp = NULL
    buf = my_strcat(buf,str1);
    temp = buf;

    buf = my_strcat(temp,str2);
    free(temp);
    temp = buf;

    buf = my_strcat(temp,str3);
    free(temp);
    temp = buf;

    buf = my_strcat(temp,str4);
    free(temp);

True about memory leaks, but is that causing that problem after about 15 iterations? — chux - Reinstate Monica, Jan 08 '15 at 19:07
@chux, good point about 15 iterations. There may be other problems in the OP's posted code. Their output doesn't match with the code. — R Sahu, Jan 08 '15 at 19:17

score 1 · Answer 3 · answered Jan 08 '15 at 19:13

Your use of buf is illogical. You haven't shown how it was allocated but even if you did malloc() memory for buf, you overwrote it with

    buf = "";

Then, you have an infinite loop with no exit condition

while(1) {
    ...    
}

which will continue to attempt concatenation until the computer catches fire. Worse, at the end of the while() loop you

free(buf);

So on the subsequent, infinite, loops, you don't even have buf to concatenate to.

String concatenate error with malloc dynamic memory allocation

3 Answers3