recv() fails to read last chunk-CodePudding

The function below reads incoming data perfectly but only if the last data chunk is smaller than BUFFSIZE. If the size of the last buff happens to be equal to BUFFSIZE, then the program tries to recv again in the next loop iteration and sets bytes to 1844674407379551615 (most probably integer overflow) and repeats this in an infinite loop... Why? Why isn't it 0? And why doesn't it escape the loop at this stage?

std::string getMsg (int clientFileDescriptor, int timeout_sec)
{
    std::string msg;
    msg.reserve(BUFFSIZE * 10);

    char buff[BUFFSIZE   1];

    signal(SIGPIPE, SIG_IGN);

    //set a timeout for reading and writing
    struct timeval tv;
    tv.tv_sec = timeout_sec;
    tv.tv_usec = 0;
    setsockopt (clientFileDescriptor, SOL_SOCKET, SO_RCVTIMEO, (const char*)&tv, sizeof(tv));


    while (true)
    {
        size_t bytes = recv (clientFileDescriptor, buff, BUFFSIZE, 0);

        std::cout << "bytes read: " << bytes << std::endl;

        if (0 < bytes && bytes <= BUFFSIZE)
        {
            msg.append(buff, bytes);

            //'buff' isn't full  so this was the last chunk of data 
            if (bytes < BUFFSIZE)
                break;
        }
        else if (!bytes) //EOF or socket shutdown by the client
        {
            break;
        }
        else
        {
            if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) 
                continue;
            
            //We cannot continue due to other erros (e.g. EBADF/socket not available)
            break;
        }
    }

    return msg;
}

CodePudding user response：

Probably size_t is unsigned on your platform. So if the recv times out, recv returns -1 and bytes overflows. Your code doesn't correctly handle this case due to the overflow.

This is just wrong:

        //'buff' isn't full  so this was the last chunk of data

It's entirely possible buff wasn't full because the last chunk of data hadn't been received yet. Tou should actually add code to detect the end of a message rather than relying on a timeout to find it for you if your application protocol supports messages of different sizes.

Unfortunately, you haven't given us the code for the other end of the connection nor specified how your application protocol works, so I have no idea what the other side does or expects. So it's not possible to give you more useful suggestions.

CodePudding user response：

If the size of the last buff happens to be equal to BUFFSIZE, then the program tries to recv again in the next loop iteration and sets bytes to 1844674407379551615 (most probably integer overflow) and repeats this in an infinite loop... Why?

Because there is no more data to read from the socket, and you are using a read timeout, so recv() fails and returns -1 (and errno will be EAGAIN or EWOULDBLOCK). recv() returns a signed ssize_t, not an unsigned size_t as you are using. size_t can't hold a negative value, so if you assign a signed -1 to a size_t, it will wrap to the largest positive value that size_t can hold.

Why isn't it 0?

Because it can't be. recv() does not return 0 on timeout, only on graceful disconnect.

And why doesn't it escape the loop at this stage?

Because recv() is returning -1. Thus, if (0 < bytes && bytes <= BUFFSIZE) and if (!bytes) are both false, so your logic falls into your final else block, which is checking errno and then continue'ing the loop on an EAGAIN or EWOULDBLOCK error (what is the point of setting a read timeout if you are just going to ignore it?). And then recv() times out and returns -1 again, and your logic continue's the loop again, over and over, forever.

TCP has no concept of message boundaries, so you have to handle this manually in your code. There are several different ways you can solve this issue:

Set a reasonable timeout, and then break the loop (and close the connection) if the timeout elapses. This is not advisable for detecting the end of a complete message, since you don't know whether the timeout elapsed because end-of-data was reached or if a network failure/hiccup occurred. Also, if the sender happens to send a new message within the timeout, you will treat it as a continuation of the previous message. So, you need a more explicit way of determining the true completion of each message, and fail accordingly on actual errors.
In which case, you can send the data's full size before sending the actual data, if you know the size up front. Then, regardless of how you read the chunks, simply stop the reading loop once you have received the specified number of bytes. Do not read more than you are told to read.
Otherwise, you can prefix each chunk with a header that specifies how many data bytes are in the chunk. After you are finished sending all of the data, send a final chunk with a data size of 0. Stop the reading loop when you receive a chunk with a data size of 0.
Otherwise, just gracefully close the socket connection after sending all of the data. Stop the reading loop when recv() returns <= 0, and process the message only if recv() had returned 0, and handle errors if recv() has returned -1 instead.