Garbage with strcpy and strcat

Question

I'm doing a client-server project in linux and I need to concatenate some strings.

I tried my code on visual studio in windows and it works fine, but it linux it gives me some garbage. I've got this function:

char* concat(char s1[], char s2[])
{
    int tam = 0;
    tam = strlen(s1);
    tam += strlen(s2);
    char *resultado =  malloc(sizeof(char) * tam) ;
    strcpy(resultado, s1); 
    strcat(resultado, s2); 
    return resultado;
}

I read that the problem is the missing of '\0' and I've done that:

 char* concat(char s1[], char s2[])
{
    int tam = 0;
    tam = strlen(s1);
    tam += strlen(s2);
    char *resultado =  malloc(sizeof(char) * tam) ;
    resultado[tam+1] = '\0';
    strcpy(resultado, s1); 
    strcat(resultado, s2); 
    return resultado;
}

The first 4 times that I called the function it works (the garbage disappeared), but then it gives me `malloc(): memory corruption

Anyone can help me?

You're allocating `tam` characters for `resultado`, and then add a `\0` character at `tam+1`. With array indices 0-based, that means you're 2 over the allocated limit. — , Jun 02 '15 at 12:29
It probably works in visual studio because it's generous, and allocates some memory beyond the limit you've set. But you can't rely on it, and running it in a debugger should throw an error. — , Jun 02 '15 at 12:31
Read about [*undefined behavior*](http://en.wikipedia.org/wiki/Undefined_behavior). — Some programmer dude, Jun 02 '15 at 12:32
If at least s1 has been `malloc()`ed before, you could **possibly** avoid copying s1 using `realloc()` with the additional length of s2 (remember to ensure there is space for an additional NUL terminator). Then just append s2 to the realloc`ed s1. — too honest for this site, Jun 03 '15 at 00:14

Iharob Al Asimi · Accepted Answer · 2015-06-02T12:36:41.907

You are not allocating space for the nul terminator, a very common mistake.

Suggestions:

Don't use sizeof(char) it's 1 by definition.
Check that malloc() did not return NULL.
Always remember the nul byte.

So your code would be fixed like this

char *resultado =  malloc(1 + tam);
if (resultado == NULL)
    pleaseDoNotUse_resultado();

Also, notice that this line

resultado[tam + 1] = '\0';

has multiple issues

tam + 1 us outside the allocated block.
You don't need to do that, strcpy() will do it for you.

Using strcat() and strcpy() in this situation is inefficient, because you already know how many bytes to copy, this

char *concat(char *s1, char *s2)
{
    size_t l1;
    size_t l2;
    char  *resultado

    if ((s1 == NULL) || (s2 == NULL))
        return NULL;
    l1 = strlen(s1);
    l2 = strlen(s2);
    resultado =  malloc(1 + l1 + l2) ;
    if (resultado == NULL)
        return NULL;
    memcpy(resultado     , s1, l1); 
    memcpy(resultado + l1, s2, l2); 

    resultado[l1 + l2] = '\0';
    return resultado;
}

would be more efficient, even when you are checking for NULL like a paranoic freak, it would be faster than strcpy() and strcat(), because you will only compute the lengths once.

Natasha Dutta · Answer 2 · 2015-06-02T12:36:40.257

You're not allocating memory to hold the termianting null. Remember, strlen() does not count the null-terminator while calculating the string length. Nevertheless, you need that space in the destination buffer to put the null terminator.

You should write

 char *resultado =  malloc(tam + 1) ;

Also,

 resultado[tam+1] = '\0';

is very wrong, because, array indexing is 0 based in C and here, you're overrunning the allocated memory (yes, even while allocating a size of tam+1 also) which will invoke undefined behaviour. You don't need that at all, you can get rid of that.

After that, as a side-note, as mentioned by Iharob also,

Check for the success of malloc() before using the returned pointer.
In C standard, sizeof(char) is guranteed to be 1. No need of using it while calculating the size for malloc().

score 0 · Answer 3 · answered Jun 02 '15 at 12:30

0

You should allocate one byte more than the resulting string's length:

char *resultado =  malloc(tam + 1) ;

Functions strcpy and strcat take care about the terminating NUL, you don't have to add it manually.

answered Jun 02 '15 at 12:30

CiaPan

9,381
2
21
35

Garbage with strcpy and strcat

3 Answers3