C C++ - TCP Socket Class : Receive Problem

Question

Hey, I did my own Socket class, to be able to send and receive HTTP requests. But I still got some problems. The following code (my receive function) is still buggy, and crashing sometimes. I tried debugging it, but it must be somewhere in the pointer arithmetics / memory management.

int Socket::Recv(char *&vpszRecvd)
{
 //vpszRecvd = NULL;
 int  recvsize = 0;
 char TempBuf[1024];
 int  Result = 0;
 char* temp;


 do
 {
  memset(TempBuf, 0, sizeof(TempBuf));

  Result = recv( this->sSocket, TempBuf, sizeof(TempBuf) -1, 0 );
  if (recvsize == 0)
   recvsize = Result;

  if ( Result > 0 )
  {
   if ( vpszRecvd != NULL )
   {
    if (temp == NULL)
    {
     temp = (char*)calloc(recvsize + 1, sizeof(char));
    }
    else
    {
     realloc(temp, recvsize + 1);
    }
    if (temp == NULL)
     return 0;

    memcpy(temp, vpszRecvd, recvsize);
    realloc(vpszRecvd, recvsize + Result);

    if (vpszRecvd == NULL)
     return 0;

    memset(vpszRecvd, 0, recvsize + Result);
    memcpy(vpszRecvd, TempBuf, Result);
    memcpy(vpszRecvd + recvsize, TempBuf, Result);
    recvsize += Result; 
   }
   else
   {
    realloc(vpszRecvd, Result);

    if (vpszRecvd == NULL)
     return 0;

    memset(vpszRecvd, 0, Result);
    memcpy(vpszRecvd, TempBuf, Result);
    recvsize += Result;
   }
  }
  else if (  Result == 0 )
  {
   return recvsize;

  }
  else //if (  Result == SOCKET_ERROR )
  {
   closesocket(this->sSocket);
   this->sSocket = INVALID_SOCKET;
   return SOCKET_ERROR;
  }
 }
 while( Result > 0 );

 return recvsize;
}

Does anybody see anything that could cause the crash, or does anyone have a better / faster / smaller and stable example how to receive a full packet via recv()?

I can't use strings, it must be done with chars, though.

Thanks for your help.

Answer 1

I had this exact problem very recently.

Realloc is slow. Recv is fast. Several hundred reallocs a second WILL crash.

calloc() not just recvsize + 1 but a buffer of a couple kilobytes extra. realloc() only when the buffer would be filled/overflow, and give it another few kilobytes extra on each realloc().

the below is a piece of code I use to append data to my output stream, but input should be very similar. (as a note, buf_out_size is the size of allocated buffer, buf_out_len is the amount of data currently in the buffer.)

    void Netconsole::ParseOutput(int sock, std::string raw)
    {


        //if too small, realloc with raw.size() + BUFFSIZE.
        if (s[sock]->buf_out_len + raw.size() > s[sock]->buf_out_size)
        {
            s[sock]->buf_out_size += raw.size() + BUFFSIZE;
            s[sock]->buf_out = (char*) realloc( s[sock]->buf_out, s[sock]->buf_out_size);
        }

        // append new data to the end of the buffer.
        if(s[sock]->buf_out != NULL)
        {
            memcpy(s[sock]->buf_out + s[sock]->buf_out_len, raw.c_str(), raw.size());
            s[sock]->buf_out_len += raw.size();

        }
        else
        {
            s[sock]->ending = true;
    #if DEBUG_PRINT_TCP
            printf("%s TCP[%d] dies from out of memory, realloc error\r\n",Debug::MTimestamp(),sock);
    #endif
        }
    }

Answer 2

You don't initialise temp and, on top of that, your call to realloc is wrong. It should be:

temp = realloc (temp, recvsize+1);

When you call realloc as you have, you throw away the new address and there's a good chance that the old address has now been freed. All bets are off when you then try to dereference it.

The reason realloc returns a new address is because expansion of the buffer may necessitate it being moved if that current block is surrounded in the memory arena (in other words, it can't just expand into a free block following it). In that case, a new block will be created in the arena, the contents transferred from the old block and the old block freed. You have to get the return value from realloc in case that happens.

Keep in mind that realloc doesn't have to return a new pointer, it may give you the same pointer if, for example, there was enough free space after the block to satisfy the new size or if you're reducing the size.

It can also return NULL if it can't expand the block, you should watch out for that as well, especially since:

temp = realloc (temp, newsize);

will result in a memory leak when it returns NULL (it doesn't free the old block).

A few other things:

• you rarely need to use calloc, especially in this case since you're copying over the memory anyway.
• similarly, you don't need to memset a memory chunk to 0 if you're immediately going to memcpy over it.
• provided you initialise temp to NULL, you can just use realloc without testing it. That's because realloc(NULL,7) is identical to malloc(7) - realloc is perfectly capable of starting with a null pointer.
• since you don't need calloc, this is for education only - sizeof(char) is always 1 by definition.
• you seem to be doing an awful lot of unnecessary copying of data.

---

Why don't we start with something a bit simpler? Now, this is totally from my head so there may be some bugs but it's at least cut down from the memory-moving behemoth in the question :-) so should be easier to debug.

It's basically broken down into:

• initialise empty message.
• enter infinite loop.
  • get a segment.
  • if error occurred, free everything and return error.
  • if no more segments, return current message.
  • create space for new segment at end of message.
  • if no space could be created, free everything and return empty message.
  • append segment to message and adjust message size.

and the code looks like this:

int Socket::Recv(char *&vpszRecvd) {
    int  recvsize = 0;
    char TempBuf[1024];
    int  Result = 0;
    char *oldPtr;

    // Optional free current and initialise to empty.

    //if (vpszRecvd != NULL) free (vpszRecvd);
    vpszRecvd = NULL;

    // Loop forever (return inside loop on end or error).

    do {
        Result = recv( this->sSocket, TempBuf, sizeof(TempBuf) -1, 0 );

        // Free memory, close socket on error.

        if (Result < 0) {
            free (vpszRecvd);
            closesocket(this->sSocket);
            this->sSocket = INVALID_SOCKET;
            return SOCKET_ERROR;
        }

        // Just return data and length on end.

        if (Result == 0) {
            return recvsize;
        }

        // Have new data, use realloc to expand, even for initial malloc.

        oldPtr = vpszRecvd;
        vpszRecvd = realloc (vpszRecvd, recvsize + Result);

        // Check for out-of-memory, free memory and return 0 bytes.

        if (vpszRecvd == NULL) {
            free (oldPtr);
            return 0;
        }

        // Append it now that it's big enough and adjust the size.

        memcpy (&(vpszRecvd[recvsize], TempBuf, Result);
        recvsize += Result;
    } while (1);
}