CODEWITHSUNDEEP
cmallocstrcpystrcat
luidgi27
luidgi27

Reputation: 324

Garbage with strcpy and strcat

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?

Upvotes: 0

Views: 737

Answers (3)

Iharob Al Asimi
Iharob Al Asimi

Reputation: 53006

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

Suggestions:

  1. Don't use sizeof(char) it's 1 by definition.
  2. Check that malloc() did not return NULL.
  3. 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

  1. tam + 1 us outside the allocated block.
  2. 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.

Upvotes: 5

Natasha Dutta
Natasha Dutta

Reputation: 3272

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,

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

Upvotes: 2

CiaPan
CiaPan

Reputation: 9571

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.

Upvotes: 0

Related Questions