CODEWITHSUNDEEP
cmemory-managementrealloc
Joe Lewis
Joe Lewis

Reputation: 978

Problems using "realloc" in C

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

Upvotes: 0

Views: 3634

Answers (5)

vincent
vincent

Reputation: 138

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
    }
}

Upvotes: 0

Adam Liss
Adam Liss

Reputation: 48290

There are two different problems.

First, the line 

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

does three things:

  1. It tries to allocate br + br_aux bytes.
  2. It may release the memory that data_aux points to.
  3. 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.

Upvotes: 2

Remy Lebeau
Remy Lebeau

Reputation: 596256

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);

Upvotes: 0

user7116
user7116

Reputation: 64068

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

  1. 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? */

  2. 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 */

  3. 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 */

  4. You're using ASCIIZ string functions on data which may not even contain chars. Use memcpy instead of strcat or strncat.

Upvotes: 1

paulsm4
paulsm4

Reputation: 121649

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

Suggestions:

  1. Make sure you include the null byte in your message before you send it

  2. Use strncat()

  3. 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".

Upvotes: 0

Related Questions