Solved

Recv() sometimes returns extra garbage

Posted on 2007-03-26
25
651 Views
Last Modified: 2010-04-15
I have a program running on Linux that sends data over a network.  One computer sends and another receives.   The incoming data is prefixed with a 4 byte integer containing the size of the message.  This allows me to specify exactly how many bytes I want to receive.  

Problem: I've noticed that very rarely, the receiving computer will receive additional garbage bytes in the receive buffer.  I'm not exactly sure why this happens.  To fix this, I used a decreasing receive buffer size on each call, to match the remaining bytes on the message, rather than the usual fixed "MAX_SIZE" value I normally pass to receive.   But I've never seen this technique done anywhere else.  Usually, people just specify some fixed "MAX_SIZE" value for the recv() function, and the recv function never sends any more data than it receives from the socket anyway.  But if I do this, I receive garbage data sometimes.

Here is what I'm doing:

int recv(int new_socket, void* buffer)
{
      uint32_t recv_size;

      /* Read 4 bytes to obtain size of recv'd data */
      switch (recv(new_socket, buffer, sizeof(uint32_t), 0)) {
            case -1: return SOCKET_ERR;
            case 0: return CLOSE_SOCKET;
            default: memcpy(&recv_size, buffer, sizeof(uint32_t));
      }

      uint32_t rem_bytes = recv_size - sizeof(uint32_t);
      uint32_t recvd_bytes;

      /* Receive rest of the data */
      do {
            switch (recvd_bytes = .recv(new_socket, buffer, rem_bytes, 0)) {
                  case -1: return SOCKET_ERR;
                  case 0: return CLOSE_SOCKET;
                  default: break;
            }
            rem_bytes -= recvd_bytes;
      } while (rem_bytes != 0);
}

Now, I don't think there's necessarily anything wrong with the above code - but I've never seen it done this way.  This is a hack I made up to fix the problem of receiving extra garbage data.  The literature on recv indicates that you should pass a constant value to the recv() function to indicate the maximum number of bytes to receive, like:

recv(sock, buffer, MAX_BYTES, 0);

...rather than the decreasing rem_bytes value I used, since recv is supposed to only receive exactly what was sent and nothing more, regardless of what value you pass as MAX_BYTES.  So why am I receiving extra garbage data with recv()?  Is this just something that happens from time to time over networks?
0
Comment
Question by:chsalvia
  • 11
  • 9
  • 4
  • +1
25 Comments
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>>       switch (recv(new_socket, buffer, sizeof(uint32_t), 0)) {
>>             case -1: return SOCKET_ERR;
>>             case 0: return CLOSE_SOCKET;
>>             default: memcpy(&recv_size, buffer, sizeof(uint32_t));
>>       }

What if recv() returns 1, 2 or 3 ? In that case, you'll read a wrong length from the buffer, and thus also a wrong message length.
0
 

Author Comment

by:chsalvia
Comment Utility
That's true...I overlooked the possibility that the initial recv could get less than sizeof(uint32_t).   I just fixed that to by adding an additional loop for the initial receive.

But the problem I was talking about above was that the recv() function was actually getting more data than I expected.  It recv'd an additional 1000 bytes or so of garbage data at the tail end of a 300K transmission.   The number of bytes it was *supposed* to receive was correctly prefixed to the buffer, and I checked the value by comparing it with how much the client actually sent.  So, the length value is correct.  The problem is recv() is receiving extra data at the end for some reason.  

Can you think of any reasons why this might happen?
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>> I just fixed that to by adding an additional loop for the initial receive.

And you still have the problem ?


>> Can you think of any reasons why this might happen?

No, recv() is not allowed to write beyond the specified buffer size. So :

    size_t recv(int socket, void *buffer, size_t length, int flags);

The third parameter (length) is the MAX. number of bytes that will be written into the buffer. There are no exceptions to that.

However, here are a few remarks/notes :

1) This line :

        uint32_t rem_bytes = recv_size - sizeof(uint32_t);

    Does that mean that the length that was sent included the leading integer itself ?
    Does the target machine have the same endianness as the source ?


2) I wouldn't name my function recv() :

        int recv(int new_socket, void* buffer)

    It will cause confusion (if not for the compiler, at least for you or anyone else reading the code).


3) I assume that the socket is a SOCK_STREAM socket ?


4) I assume that the . in this line is a typo :

        switch (recvd_bytes = .recv(new_socket, buffer, rem_bytes, 0)) {


5) You're constantly overwriting the buffer. Use something like this instead :

        char *buffer_ptr = buffer;
        do {
            switch (recvd_bytes = recv(new_socket, buffer_ptr, rem_bytes, 0)) {
                  case -1: return SOCKET_ERR;
                  case 0: return CLOSE_SOCKET;
                  default: break;
            }
            buffer_ptr += recvd_bytes;
            rem_bytes -= recvd_bytes;
        } while (rem_bytes != 0);

    Note that I don't check whether the buffer is big enough - that's something you'll have to add.


6) If I were you, I would return the length of the data that was read. Maybe place this at the end of your function :

        return (recv_size - sizeof(uint32_t));
0
 
LVL 22

Assisted Solution

by:grg99
grg99 earned 50 total points
Comment Utility
There is no way recv() can "read" "extra" data.  

Are you sure you're not sending extra data?  

Also you need to carefully read the man page for recv().  There is no guarantee you're going to get four bytes, or sizeof( uint32 ), on the first read.  You need to check the return value from recv() to ensure you've gotten all the bytes you  need to get a valid length.

Also as other have noted, if the receiving computer orders bytes differently, the count will be wildly wrong..  The only safe way is to send the byte count in some portable way, such as ASCII.

 
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> What if recv() returns 1, 2 or 3 ? In that case
recv returns the number of bytes received or socket error (-1). If bytes received is 0 the socket was closed. 1, 2, or 3 bytes is only possible, if the sender would send a buffer of that size. A tcp/ip message never was truncated at an odd byte number. If there is only one sender that always sends at least 4 bytes or more the above cases must not be handled.

>>>>> 5) You're constantly overwriting the buffer.
I would say that is the bug. Together with the missing length check.

The second read loop never would receive all data if recv was called twice. The size passed to recv was rem_bytes what isn't the buffer size but the expected size to receive. Normally recv would get all data with the first call. But if lengthy data it might return less, what means that the second recv overwrites the buffer which was filled with the data of the first call. I think the final result looks like it has garbage at the end of the buffer but actually the buffer only contains the second part of the data transmitted and the garbage comes from a missing initialization.

Regards, Alex
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>> 1, 2, or 3 bytes is only possible, if the sender would send a buffer of that size. A tcp/ip message never was truncated at an odd byte number.

How do you know where the sender truncates the packets ? How do you know whether a packet contains part of the previous message, and 1, 2 or 3 bytes of the int of the next message ?
Same questions for the TCP/IP internal buffer on the receiving end.

You can't. Even though you won't see it go wrong often, you cannot rely on receiving all 4 bytes at once. So, you need to explicitly check that you actually received the 4 bytes you requested.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>>> How do you know where the sender truncates the packets ?
I do know that there is no TCP/IP subsystem with odd buffer lengths or odd package lengths. So, the only way to get a buffer size of 1, 2, or 3 is if the sender makes a send(sock, buffer, 3) what can be excluded if you write both sender and receiver.

It's a similar thing like

  enum MonAbb { JAN=1, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV,DEC };
  private:
   string MyDateClass::getMonthAbbr(MonAbb month)
   {
            static string months = "JANFEBMARAPRMAYJUNJULAUGSEPOCTNOVDEC";
            return months.substr((month-1)*3, 3);  
   }

Of course you could check for month is between 1 and 12 and string length is 36.  But the enum the check simply was redundant. Moreover the additionally if statement can buggy as well , e. g. if (month > 1 && month <= 12), and you will find it out next january.

In the case above it is not as simple: but if you have a communication with one sender only which always sends 4 bytes or more, you can check for number of bytes received is 1, 2, or 3 but I bet that these statements never were reached.

>>>> you cannot rely on receiving all 4 bytes at once
Theoretically I can't, practically I can. I would say the chance that I get 1,2,3 bytes is much less than the chance that I make errors when handling that case. You may have other experiences but I made a lot of code reviews and there was a big  number of bugs I found in error handling code most of them redundant or copied by another source. the problem with that is that only a tiny part of the error handling code ever was processed so the bugs mostly keep unknown. Nevertheless they make the code less readable.

Regards, Alex
 
P.S. Can we agree on the following?

  int received = recv(...);
  if (received >= 4)         // ok
  else if (received == 0)   return CLOSE_SOCKET;
  else                               return SOCKET_ERROR;

0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
Alex, the main reason I mentioned that you can't be sure that you actually receive 4 bytes, is because it COULD explain the problem he's having (my first post was only after quickly scanning his code).
My later post included more things that I noticed could explain the problem.

>> Theoretically I can't, practically I can.
I've learned the hard way that this can be dangerous in certain cases.
In this case, the sender is sending a lot of messages in a row, with decreasing lengths. If the packet size is configured to be 1500 eg., then it wouldn't surprise me that at some point, the boundary between two messages packed in the same packet is just before the 1500 byte packet boundary.
ie. message x ends at byte 1498 in the packet, and thus the first two bytes of message x+1 are put in the same packet, and the rest of the message in the next packet. I'm not just making this up :)

The reason that it's unlikely that this happens, is because usually, the network between sender and receiver is sufficiently fast, so that both packets in question will arrive at the receiver one after the other, probably before the next call to recv() can be done. But beware of delays on the network :)

>> P.S. Can we agree on the following?
That's indeed what I would have done : instead of the "default : break;" I would have used "4 : break;".
It's not more complicated (ie. no new bugs introduced by adding error checking code as you said), but it does solve a possible future bug (however unlikely).



Anyway, as I said : this was only my first thought after quickly scanning the code. Now, I agree with you that the problem is most likely the buffer that gets overwritten every time.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> the problem is most likely the buffer that gets overwritten every time.
Bingo. Here we have a bug that obviously would lead to the wrong result if recv didn't receive the whole message with one read.

>>>> message x ends at byte 1498 in the packet, and thus the
>>>> first two bytes of message x+1 are put in the same packet,
Sorry that I am insistent but that cannot happen as any message was fully read by the function above.  As long as the size written to the first bytes of the message was correct the rest of the message was read by the while loop and no pending rest can spoil the next message.

By writing that I see another error in the initial code. If the sender would send a second message immediately after the first so that two messages were to read, the while loop might read parts of the second message or the whole message ... and would run into a blocking recv after that cause the while condition never was true. I assume that never happened till now maybe because the sender needs too much time between two messages or because there is some feedback before sending new messages.

chsalvia, you need to change the while loop, e. g. by taking the code that Infinity has posted above (5). You should consider to always allocating a new buffer after reading the needed size. Then you don't need to bother for the maximum buffer size.

Regards, Alex
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
Maybe like that:

  // make it a dynamically sized buffer
  int  readMsg(int new_socket, unsigned char** buffer, int* bufsize)
  {
        unsigned char *buffer_ptr = NULL;
        int rem_bytes = 0;    // make it an int so that you can check for <= 0
        int recvd_bytes = 0;

        ...

        bufsize = recv_size - sizeof(uint32_t) ;
        *buffer = (unsigned char*)malloc(bufsize ) ;
        buffer_ptr = *buffer;
        do {
            switch (recvd_bytes = recv(new_socket, buffer_ptr, rem_bytes, 0)) {
                  case -1: return SOCKET_ERR;
                  case 0: return CLOSE_SOCKET;
                  default: break;
            }
            buffer_ptr += recvd_bytes;
            rem_bytes -= recvd_bytes;
        } while (rem_bytes > 0);

        return bufsize;
   }
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>> Sorry that I am insistent but that cannot happen ...

I didn't explain myself very well apparently. Suppose the sender sends 2 messages without a pause between the two. Both messages are 1498 bytes long. The TCP stack might combine these in the same packet (as much as possible). So, these two packets might be sent :

    packet 1 : the entire first message (1498 bytes) + the first 2 bytes of the second message
    packet 2 : the rest of the second message (1496 bytes)

What if the second packet is not sent correctly for example ? What if it doesn't arrive in time, and the recv() returns with only 2 bytes read for the second message (the two bytes that are in packet 1) ?

As I said before : it's probably not the problem here (and it's not very common), but it is a possibility.


>> the while loop might read parts of the second message or the whole message

That's why the length was sent first, and that's why chsalvia used rem_bytes as parameter to recv() - to make sure that only one message is read at a time.
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>> Maybe like that:
>>
>> // make it a dynamically sized buffer
>> <SNIP>

That's a very good idea indeed !
0
Do You Know the 4 Main Threat Actor Types?

Do you know the main threat actor types? Most attackers fall into one of four categories, each with their own favored tactics, techniques, and procedures.

 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> the recv() returns with only 2 bytes read for the second message
The recv may return but the while loop wouldn't end before not all bytes *promised by the initial size value* were read. A second message would make that recvd_bytes was greater than rem_bytes and cause an integer overflow for rem_bytes (as long it is unsigned and the condition was 'rem_bytes  != 0'). Then the next recv probably would block and the while loop never breaks (in that life or before disconnect).
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>> The recv may return but the while loop wouldn't end

The point is that originally, there WAS no loop :) ... quoting :

      /* Read 4 bytes to obtain size of recv'd data */
      switch (recv(new_socket, buffer, sizeof(uint32_t), 0)) {
            case -1: return SOCKET_ERR;
            case 0: return CLOSE_SOCKET;
            default: memcpy(&recv_size, buffer, sizeof(uint32_t));
      }
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> That's why the length was sent first

Sorry I was wrong with  "recvd_bytes was greater than rem_bytes" cause that never can happen. But the while loop nevertheless would not end until the mesagge was fully read (or an error occurs).

 

0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> The point is that originally, there WAS no loop :) ... quoting

Yeah, but now we come back to the first disagreement that tcp/ip would cut a *new* message within the first bytes at an *odd* byte boundary ...
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
>> Yeah, but now we come back to the first disagreement that tcp/ip would cut a *new* message within the first bytes at an *odd* byte boundary ...

quote from RFC 1180 :

   "TCP packetizes the byte stream at will; it does not retain the
   boundaries between writes.  For example, if an application does 5
   writes to the TCP port, the application at the far end might do 10
   reads to get all the data.  Or it might get all the data with a
   single read.  There is no correlation between the number and size of
   writes at one end to the number and size of reads at the other end."

Note the "at will" and "no correlation" parts. Also remember that the size of the sent packets is not necessarily the same as the received packets (they might have been split up or even combined along the way).

There is no guarantee that it happens as you say, although I agree that it usually (read : in "not unusual" circumstances) will.

btw, sorry chsalvia that we're sidetracking a bit on the implementation details of TCP/IP protocol stacks :)
0
 

Author Comment

by:chsalvia
Comment Utility
Infinity08, itsmeandnobodyelse, grg99:

Thanks for all your comments.  It's been very helpful reading your exchanges.  Although it's probably very unlikely that the initial recv() would ever get less than 4 bytes, it seems to me that a really robust program would account for that possibility anyway, no matter how slim the chances are.  So I wrote in an initial loop.

Now, the consensus here seems to be that the problem is buffer overwrites.  And here I must apologize, because actually I did take that into account.  I simplified my posted code a bit.   The reason I did that was because I originally did this with a dynamically growing C++ buffer class - but I wanted to post in the C area because the issue with send/recv is really C related.  Perhaps that was a bad idea, as it led to a misdiagnosis of the problem.

The code I use is: (including the new initial loop):

uint32_t recv_size;

/* Read 4 bytes to obtain size of recv'd data */
do {
      switch(buffer.recv(new_socket, sizeof(uint32_t))) {
            case -1: return SOCKET_ERR;
            case 0: return CLOSE_SOCKET;
            default: break;
      }
} while (buffer.used_bytes < sizeof(uint32_t));

memcpy(&recv_size, buffer.data, sizeof(uint32_t));

/* Now receive the rest of the data */

do {
      switch(buffer.recv(new_socket, recv_size)) {
            case -1: return SOCKET_ERR;
            case 0: return CLOSE_SOCKET;
            default: break;
      }
} while (buffer.used_bytes < recv_size);


And buffer.recv() is defined as:

inline int recv(int m_sock, uint32_t maxrecv, int flags = 0) {
      if (used_bytes + maxrecv > size) buffer_realloc(used_bytes + maxrecv);
      int status = ::recv(m_sock, (byte*) data + used_bytes, maxrecv, flags);
      if (status > 0) used_bytes += status;
      return status;
}

So, the buffer grows dynamically as it receives more data.  

Also, the problem still occurs even after I added in the initial loop to get the 4 bytes.  So, would it be safe to conclude that the problem has to be something on the send() side?
0
 

Author Comment

by:chsalvia
Comment Utility
I should also mention that I'll change the code so that the send side sends the length in ASCII format, rather than an integer, to account for differing endianess.  However, that is not the immediate problem here, because both machines are little endian.
0
 
LVL 53

Accepted Solution

by:
Infinity08 earned 300 total points
Comment Utility
Try this instead :

uint32_t recv_size_tmp = recv_size;
/* make sure that buffer.used_bytes == 0 */
do {
      int status = 0;
      switch(status = buffer.recv(new_socket, recv_size_tmp)) {
            case -1: return SOCKET_ERR;
            case 0: return CLOSE_SOCKET;
            default: recv_size_tmp -= status; break;
      }
} while (buffer.used_bytes < recv_size);


Or something similar. Basically, the maxrecv parameter passed to buffer.recv() has to have the correct value (ie. the max. you want to receive).
0
 
LVL 53

Expert Comment

by:Infinity08
Comment Utility
You could also put all of that inside the buffer.recv() function ... that's maybe a bit nicer ...
0
 

Author Comment

by:chsalvia
Comment Utility
Infinity08,

>>Or something similar. Basically, the maxrecv parameter passed to buffer.recv() has to have >>the correct value (ie. the max. you want to receive).

That's what I tried originally.  That makes sense, but it doesn't seem to correspond to what the literature on recv() says.  As grg99 said above, "there is no way recv() can "read" "extra" data".  Even if I pass a max value of 1000000000 bytes, if only 15 bytes were sent then only 15 bytes will be received.  The value only specifies the space available - it doesn't effect how much is actually received.  So I shouldn't even need to keep decrementing recv_size_tmp.  Recv is only supposed to recv the bytes that were sent.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> "there is no way recv() can "read" "extra" data"
if the sender sent data correctly the receiver never is requesting extra data but only the data which may being received delayed but always must come.

>>>> while (buffer.used_bytes < recv_size)
don't use < with unsigned integers that were result of a subtraction. I personally never use unsigneds other than for the buffer data.

Regards Alex

 
0
 
LVL 53

Assisted Solution

by:Infinity08
Infinity08 earned 300 total points
Comment Utility
>> As grg99 said above, "there is no way recv() can "read" "extra" data".

The receiver can only receive what has been sent by the sender. But if the sender sends 2 messages, they might both be in the receiver's TCP/IP buffer already when you call recv(). So, if you ask more than the size of the first message, you might get part of the second message already.

That's not necessarily bad if you keep your own buffer though !!



>> That's what I tried originally.  That makes sense, but it doesn't seem to correspond to what the literature on recv() says.

I'm not sure what you mean ... I guess the literature you refer to, uses their own buffer on the receiver side, and just fills the buffer, and then reads messages from the buffer. Can you clarify what you mean ?
0
 
LVL 39

Assisted Solution

by:itsmeandnobodyelse
itsmeandnobodyelse earned 150 total points
Comment Utility
>>>> That's not necessarily bad if you keep your own buffer though !!
In the prog above it is bad cause each message necessarily should be read separatedly so that the first recv could extract the message size says

>>>> That makes sense, but it doesn't seem to correspond to what the
>>>> literature on recv()
I think it is a misunderstanding.

chsalvia, you are right by stating that you can't receive more than has been sent but as far as the sender is not lying, i. e. it sent exactly the number of bytes that was proposed within the first 4 bytes, you can make it sure to read exactly one message with one or more calls of recv. You can do it by  providing exactly a buffer of that size that is needed to take the remaining rest of the message, not less and not more. That is the precondition for the next recv call(s)  to get the next message starting with the message size again.

Regards, Alex

0

Featured Post

How your wiki can always stay up-to-date

Quip doubles as a “living” wiki and a project management tool that evolves with your organization. As you finish projects in Quip, the work remains, easily accessible to all team members, new and old.
- Increase transparency
- Onboard new hires faster
- Access from mobile/offline

Join & Write a Comment

Have you thought about creating an iPhone application (app), but didn't even know where to get started? Here's how: ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ Important pre-programming comments: I’ve never tri…
Windows programmers of the C/C++ variety, how many of you realise that since Window 9x Microsoft has been lying to you about what constitutes Unicode (http://en.wikipedia.org/wiki/Unicode)? They will have you believe that Unicode requires you to use…
The goal of this video is to provide viewers with basic examples to understand and use structures in the C programming language.
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use nested-loops in the C programming language.

744 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

8 Experts available now in Live!

Get 1:1 Help Now