Problems using "realloc" in C

Question

I have code receiving string data from a socket which crashes on the first iteration:

int size_data = 1024*sizeof(char);              
char *data = malloc(size_data);
char *data_aux;
int br_aux=0;
int *nptr;

memset(&data[0], 0, size_data);
int br = recv(sockdata, data, size_data, 0);
data_aux = malloc(br);
while (br>0) {
  br_aux = br_aux + br;
  strcat(data_aux, data);
  br = recv(sockdata,data, size_data, 0);
  if (br > 0) {
    nptr = (int *) realloc(data_aux, br+br_aux);
  }
}
free(data);
printf("%s", data_aux);
free(data_aux);

Nothing so complicated, but however I get an error:

* glibc detected ./clientFTP: free(): invalid next size (normal): 0x00000000061d6420 ** ======= Backtrace: ========= /lib64/libc.so.6[0x366be7247f] /lib64/libc.so.6(cfree+0x4b)[0x366be728db] ./clientFTP[0x401e39] /lib64/libc.so.6(__libc_start_main+0xf4)[0x366be1d9b4] ./clientFTP[0x400b89] ======= Memory map: ======== 00400000-00403000 r-xp 00000000 fd:00 5396214 /home/alumno/FTP/clientFTP 00602000-00603000 rw-p 00002000 fd:00 5396214
/home/alumno/FTP/clientFTP 061d6000-061f7000 rw-p 061d6000 00:00 0
[heap] 366ba00000-366ba1c000 r-xp 00000000 fd:00 1994999
/lib64/ld-2.5.so 366bc1c000-366bc1d000 r--p 0001c000 fd:00 1994999
/lib64/ld-2.5.so 366bc1d000-366bc1e000 rw-p 0001d000 fd:00 1994999
/lib64/ld-2.5.so 366be00000-366bf4e000 r-xp 00000000 fd:00 1995001
/lib64/libc-2.5.so 366bf4e000-366c14e000 ---p 0014e000 fd:00 1995001
/lib64/libc-2.5.so 366c14e000-366c152000 r--p 0014e000 fd:00 1995001
/lib64/libc-2.5.so 366c152000-366c153000 rw-p 00152000 fd:00 1995001
/lib64/libc-2.5.so 366c153000-366c158000 rw-p 366c153000 00:00 0 3672200000-367220d000 r-xp 00000000 fd:00 1995011
/lib64/libgcc_s-4.1.2-20080825.so.1 367220d000-367240d000 ---p 0000d000 fd:00 1995011
/lib64/libgcc_s-4.1.2-20080825.so.1 367240d000-367240e000 rw-p 0000d000 fd:00 1995011
/lib64/libgcc_s-4.1.2-20080825.so.1 2b5cdf8d9000-2b5cdf8dd000 rw-p 2b5cdf8d9000 00:00 0 2b5cdf8f6000-2b5cdf8f7000 rw-p 2b5cdf8f6000 00:00 0 7fffae47e000-7fffae493000 rw-p 7ffffffe9000 00:00 0
[stack] 7fffae5fc000-7fffae600000 r-xp 7fffae5fc000 00:00 0
[vdso] ffffffffff600000-ffffffffffe00000 ---p 00000000 00:00 0
[vsyscall] Aborted

Adam Liss · Accepted Answer · 2012-05-16T02:12:51.187

2

There are two different problems.

First, the line

nptr = (int *)realloc(data_aux,(br+br_aux));

does three things:

It tries to allocate br + br_aux bytes.
It may release the memory that data_aux points to.
It points nptr to the address of the newly-allocated memory.

But the code continues to use data_aux as if it still points to the new memory.

Second, since recv() returns the number of bytes that were received, you should use that information to append data to the buffer:

while (br > 0) {
  memcpy(data_aux + br_aux, data, br);
  br_aux += br;
  br = recv(sockdata, data, size_data, 0);
  if (br > 0) {
    nptr = (int *) realloc(data_aux, br + br_aux);
    if (nptr == NULL) {
      // ERROR
    }
    data_aux = nptr;
  }
}

The problem with recv() in combination with any of the string operations (like strcat()) is that recv() can return binary data. If that data happens to contain a zero byte, the string functions will assume it's the end of the data and behave in ways you neither expect nor want.

There's actually a third problem, which is that none of the return values is checked for errors. Always check for errors instead of assuming that memory is valid, or that socked communication has succeeded.

edited May 16 '12 at 02:12

answered May 16 '12 at 01:59

Adam Liss

47,594
12
108
150

Thank you very much it worked !!! But I just changed your code in this line: nptr = (int *) realloc(data_aux, br + br_aux); for this one: data_aux = (char *) realloc(data_aux, br + br_aux); Could you explain why memcpy worked instead of strcat?? Thanks – Joe Lewis May 16 '12 at 02:06
Glad it worked, but you really should check for errors. `realloc()` can return `NULL` if an error occurs, and that will make the program crash. You want `memcpy()` instead of `strcat()` because string functions will stop when they encounter a 0 (null character) byte. The `memxxx()` functions take an explicit length argument. – Adam Liss May 16 '12 at 02:12
and what could I use if I want to receive other kind of files like a zip file (I tried but it didn't work) – Joe Lewis May 16 '12 at 02:31
1

Saying "it didn't work" is too vague. What happened? The snippet above should work for any sort of file, unless you run out of memory ... which is why you need to check the return values for errors. :-) – Adam Liss May 16 '12 at 02:37
Apparently I received 1024 bytes several times in br (since the file is big) but i don't know why it doesn't copy the information received by the socket. The result is a 5 byte data, when the original file is 19xxx bytes. What could I check? – Joe Lewis May 16 '12 at 02:49
Are you still using `printf()` to display the data? Just like the other string functions, it will stop when it encounters the first 0 byte. – Adam Liss May 16 '12 at 02:54
I have printed the data with printf, and it returns a lot of strange characters (since it's a zip file), however the first data returned is the final data i get (the 5 byte data) so it ignores the concatenation of the rest of the data... – Joe Lewis May 16 '12 at 03:02
You might try printing `data` and `data_aux` _inside_ the loop, so you can see the data as you receive it. Also beware that, since the loop will terminate before the final `memcpy()`, you'll lose the last buffer that's received. (You can either alter the loop or add another `memcpy()` after the loop.) – Adam Liss May 16 '12 at 03:06
The problem is the method of copy, memcpy doesn't copy more information just the first 5 bytes returned before the loop, and strcat doesn¡t fill data_aux completely (but 8094 bytes), however it finishes correctly and without errors.... :\ – Joe Lewis May 16 '12 at 03:20

user7116 · Answer 2 · 2012-05-16T02:03:58.210

1

There are many problems in the code, but I'll tackle the big ones:

You're allocating memory and assuming it is ready for use:

data_aux = malloc(br);
...
strcat(data_aux, data); /* <- who said data_aux isn't garbage? */

You should consider the case where realloc has moved your data or when the call itself fails and data_aux isn't big enough:

Upon successful completion with a size not equal to 0, realloc() returns a pointer to the (possibly moved) allocated space. If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() is returned. If there is not enough available memory, realloc() returns a null pointer and sets errno to ENOMEM.
```
data_aux = realloc(data_aux, br + br_aux); /* reassign data_aux */
```
You're not checking any return values. A big one is not checking the result of recv() prior to allocating memory with it:
```
br = recv(...);
...
data_aux = malloc(br); /* -1 as a size is large in unsigned speak */
```
You're using ASCIIZ string functions on data which may not even contain chars. Use memcpy instead of strcat or strncat.

edited May 16 '12 at 02:03

answered May 16 '12 at 01:55

user7116

63,008
17
141
172

To clarify the second point, you should be assigning the return value of realloc to data_aux, not to nptr. (And you should be casting the return value to `char*`.) – happydave May 16 '12 at 01:57
This answer misses _both_ of the big problems. There's no check for null characters, and the pointer is never adjusted to point to the re-allocated memory. It's _never_ acceptable to call `realloc()` and continue to use the original pointer. – Adam Liss May 16 '12 at 02:04
@AdamLiss: I'll clarify "ready for use" and add more big ones. – user7116 May 16 '12 at 02:04
Thanks you very much you are right !! As the user Adam Liss proposed, I used memcpy instead of strcat, what I suppose initialize the info of data_aux in case of containing garbage... isn't it? – Joe Lewis May 16 '12 at 02:09
@JoeLewis: you only care if the size of that buffer is not the same size as the data received by `recv`. If this is ASCIIZ string data this means you need to ensure it is NUL terminated or copy it into another properly sized char buffer. – user7116 May 16 '12 at 02:11

paulsm4 · Answer 3 · 2012-05-16T01:59:49.537

0

Uh - is the string you receive from the socket null-terminated?

Suggestions:

Make sure you include the null byte in your message before you send it
Use strncat()
Null terminate the buffer yourself, if necessary

PS: I'm guessing that you're overrunning the buffer in your "strcat(data_aux)", which is inadvertantly corrupting both "data" as well as "data_aux".

edited May 16 '12 at 01:59

answered May 16 '12 at 01:47

paulsm4

114,292
17
138
190

1

I don't see how that can solve my problem, the concatenation is supposed to work fine with strcat, and the string received from the socket is not null since I use the br variable. – Joe Lewis May 16 '12 at 01:57

score 0 · Answer 4 · answered May 16 '12 at 02:12

If realloc() succeeds, the input memory block is no longer valid, and the returned pointer is pointing to the new memory block. If realloc() fails, the input memory block is still valid. You are assigning the returned pointer to an nptr variable that is not used for anything at all, and you never update the data_aux variable to point at the reallocated memory. When you call free() to free data_aux, if realloc() had been called beforehand and was successful, you would be freeing the wrong pointer, which can lead to crashes like you are seeing.

Change this:

nptr = (int *) realloc(data_aux,(br+br_aux));

To this instead:

nptr = (char*)realloc(data_aux, br_aux + br); 
if (!nptr)
{
    ... error handling ...
    break;
}
data_aux = nptr;

With that said, you should re-write the entire logic to something more like the following instead. There is no need to call recv() multiple times in the loop:

int size_data = 1024 * sizeof(char);                
char *data_aux = NULL;
int br_aux = 0;  
char *nptr;  

char *data = malloc(size_data);  
if (data)
{
    int br = recv(sockdata, data, size_data, 0);
    while (br > 0)
    {  
        nptr = (char*) realloc(data_aux, br_aux + br);
        if (!nptr)
            break;

        data_aux = nptr;
        memcpy(&data_aux[br_aux], data, br);  
        br_aux = br_aux + br;  
    }  

    free(data);  
}

printf("%.*s", br_aux, data_aux);  
free(data_aux);

To simplify it further, put the data buffer on the stack instead:

char* data_aux = NULL;
int br_aux = 0;  

char data[1024];
char *nptr;  

int br = recv(sockdata, data, sizeof(data), 0);
while (br > 0)
{  
    nptr = (char*) realloc(data_aux, br_aux + br);
    if (!nptr)
        break;

    data_aux = nptr;
    memcpy(&data_aux[br_aux], data, br);  
    br_aux = br_aux + br;  
}  

printf("%.*s", br_aux, data_aux);  
free(data_aux);

score 0 · Answer 5 · answered May 16 '12 at 02:39

data_aux = malloc(br);   //data_aux may not filled with zero
//data_aux[0] = '\o';

while (br>0)
{
    br_aux = br_aux + br;  
    strcat(data_aux,data); //this may casue overflow, data may not end with '\0'
    br = recv(sockdata,data, size_data,0); 
    if(br>0)
    {
        //need check nptr is NULL? and size is (br + br_aux + 1), '\0' need one byte
        //nptr = (char*)realloc(data_aux,(br + br_aux + 1)); 
        nptr = (int *)realloc(data_aux,(br+br_aux)); 
        // if (nptr != NULL)
        //     data_aux = nptr;
        // else
        //     Error handling
    }
}

Problems using "realloc" in C

5 Answers5

Linked