C Socket Append Data To Buffer Gets Corrupted

Question

I'm still refining my C coding skills and keep running into issues with properly managing memory--go figure. Anyhow, I'm reading from a socket and I'm fine as long as my total response length from the socket it no bigger than my buffer size. I know this because when I increase the buffer size large enough for the incoming data, it works just fine for the larger payloads. Creating a really large "just-in-case" buffer on the stack isn't feasible, obviously, so I want to grow the buffer dynamically on the heap. Here's what I'm doing currently:

raw_response = NULL;

// Receive from the Web server
retcode = recv(server_s, in_buf, BUF_SIZE, 0);

while ((retcode > 0) || (retcode == -1))
{
  totalLength += retcode;

  if (raw_response == NULL) {
    raw_response = (char*)malloc(sizeof(char)*totalLength);
    memcpy(raw_response, in_buf, totalLength);
  } else {
    raw_response = (char*)realloc(raw_response, sizeof(char)*totalLength);
    memcpy(raw_response+previousLength, in_buf, retcode);
  }

  previousLength = retcode;
  retcode = recv(server_s, in_buf, BUF_SIZE, 0);
  if (retcode == 0 || retcode == -1) {
    printf("\n\nNo more data, bailing. Data length was: %lu\n\n", totalLength);
  }
}

If the raw_response is NULL, I know I have not received any data yet, so I use malloc. Otherwise, I use realloc so that I don't have to build up a new buffer. Instead I can just append the incoming data. So to get the end of the existing data after the first iteration, I take the address of raw_response and add the previous length to that and append the new data there assuming it's correctly appending on each subsequent call to recv().

The problem is that my final buffer is always corrupted unless I change BUF_SIZE to something larger than my total incoming data size.

Seem like it's probably just something simple I'm overlooking. Any thoughts?

If you have a lot of messages alloc memory in chunks (512 bytes, 1024) so you dont have to keep allocating resources. did you checked if the size have the \0 included? — Lefsler, Jun 19 '13 at 18:23
[Please do not cast the result of malloc](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). Also, `previousLength = retcode` is wrong, you want `totalLength`. Also, if you get `-1`, that's an error, it doesn't mean you have to return a byte to the byte bank (which you are not doing correctly), it means you need to bail out now. — n. m. could be an AI, Jun 19 '13 at 18:28
@n.m. Thanks for the aside. I've done that, but this is cross-platform C code and Visual Studio complains when I don't cast. I would guess there's some way to make it stop complaining, but I didn't look into it. — Matt Long, Jun 19 '13 at 18:34
@n.m. Can you make your comment into an answer? The line with `previousLength = retcode;` was the problem. I'd like to give you credit. That's exactly what I was hoping for--something simple that I was overlooking. I really appreciate it. — Matt Long, Jun 19 '13 at 18:36
@n.m. On a crusade are we? ;-) I'll see what I can find. Thanks again for your help. — Matt Long, Jun 19 '13 at 18:53
@n.m. From what I can tell, the Visual Studio compiler complains because it is actually a C++ compiler which requires a cast. Are you aware of any other reasons? There doesn't appear to be a setting to disable the warning. — Matt Long, Jun 20 '13 at 15:47
MSVC should compile files with ".c" extension in C mode, no C++ mode. You might just as well switch to C++. In C++ you *must* use this cast, so yeah. Whatever suits you. Note that C++ is a *very* different language from C. Good C programs are often bad C++ programs, even when they are valid C++ programs! — n. m. could be an AI, Jun 20 '13 at 16:47

score 2 · Accepted Answer · edited May 23 '17 at 10:25

The problem is these lines:

memcpy(raw_response+previousLength, in_buf, retcode);
previousLength = retcode;

Your function will work for the first and second iterations but after that will start corrupting data. I assume you meant to write previousLength += retcode;

There are some other problems with the code which aren't the answer to your question. Firstly, what happens if realloc or malloc fails? You don't check for this in your little sample. Also, you can always just use realloc (which will act like malloc if the pointer is NULL see this SO question). i.e.

char *tmp = realloc(raw_response, sizeof(*tmp) * totalLength);
if (tmp == NULL)
     return -ENOMEM;
raw_response = tmp;
memcpy(raw_response + previousLength, in_buf, ret_code)

Secondly, you might call memcpy when ret_code is -1 (also changing totalLength by -1 which will again cause problems).

I appreciate the code review. Thanks for the additional suggestions. I will implement them. I am waiting to see if n.m. will convert his comment (in the question) to an answer so I can award it to him since he spotted the actual problem first and responded first. I'll give him a couple days to do so. If he doesn't do so soon, I'll award it to you. Thanks again for your input. — Matt Long, Jun 20 '13 at 15:34

C Socket Append Data To Buffer Gets Corrupted

1 Answers1