Thread: Queue Problem

Hello,

I'm having a little trouble with some code I've written that receives a variable number of data objects from a client. The objects are not represented in this code; The data received is merely buffered and passed to a callback function that returns:-
-2 if it has encountered a marker indicating the end of the list;
-1 if it encountered an error;
0 if it has not read any data (waiting for more);
Or a positive number of bytes read.

The code is based around filling a 512 byte queue-style buffer with data and moving a write pointer within it depending on how much data is received.
Unfortunately I have encountered a few situations now where ulSize, which is supposed to describe to the callback function how many bytes in the buffer is available to it, exceeds the maximum length of the buffer (512), which is naturally very bad.
It never happens when there is a debugger attached, which suggests that there is a very specific set of circumstances which causes the problem.

I know it's probably not the best implementation in the world but it (almost) does the job. If anyone could suggest improvements I'd be grateful.

Code:

// snippet, here's the variables used
int iRet, iRet2;
unsigned char lpBuffer[512], *lpPtr;
unsigned long ulSize;
SOCKET sockClient;

// initialize queue stuff
lpPtr = lpBuffer;
ulSize = 0;
while (1)
{
    // receive data into the queue beginning at the write pointer
    iRet = recv(sockClient, lpPtr, (lpBuffer + 512) - lpPtr, 0);
    if (!iRet || iRet == SOCKET_ERROR)
        iRet = -2;  // transmission error
    else
    {
        ulSize += iRet; // add the length of the new data to the total in the queue
        if (ulSize > 512)
            WriteLog("What the...?!?\r\n");  // this appears in the log when bad things happen

        while (ulSize) // there is data in the queue
        {
            // pass the received data to the callback function
            iRet2 = g_lpCommand.fpCallback(ulSize, lpBuffer);
            if (iRet2 == -1)  // error
            {
                iRet = -2; // transmission error
                break;
            }
            else if (iRet2 == -2) // complete
            {
                iRet = -3; // complete
                break;
            }

            if (iRet2) // a positive number was returned from the callback (bytes read)
            {
                ulSize -= iRet2; // subtract from the total
                if (ulSize) // if some is left, move it to the front of the queue
                    memmove(lpBuffer, lpBuffer + iRet2, ulSize);

                // if some is left, position the write pointer after it, else point to the front
                if (ulSize < 512)
                    lpPtr = lpBuffer + ulSize;
                else
                    lpPtr = lpBuffer;

            }
            else // nothing read, get more data
                break;

        }

    }

    if (iRet < 0) // there was an error, stop receiving
        break;

}

if (iRet == -3) // clear error if it indicates completion
    iRet = 0;

The variable ulSize is only modified in two places, after recv() and after the callback.
There is little chance that recv() returns something greater than the size indicated
(<=512) so let's focus on the callback part: what if it returns a value greater to 512?
(I know this is not in the specification but it may be a bug) there is an underflow in
this case. Or perhaps the size indicated to recv() is greater than 512 (if lpPtr is
incorrectly set at some point) leading to a corruption of ulSize later...

root4: AFAIK the callback function can't return a value greater than 512. It bases its return value on the size specified in the object, checking beforehand that that doesn't exceed the size of the buffer passed to it. The only time that this can exceed 512 is when the size parameter passed to it exceeds 512, which indicates a problem outside of the callback.

Originally Posted by swoopy

Shouldn't you stay in the inner loop here, instead of breaking out. At this point iRet2 will equal 0, and ulSize will be greater than 0. Then at the top you have ulSize += iRet, which will likely make ulSize greater than 512.

You could also reset ulSize back to 0 at the top of the outer while(), but in this case the callback would not have read all the data.

That's not the top of the loop though? At the top a recv() occurs, which should put more data into the queue at the location of the write pointer. So, there would then be more data in the queue and ulSize would be updated as expected.

Let's say that the callback function is expecting a stream of 28-byte objects to come from the client. The first recv() hypothetically returns 20, so the callback would return 0, and the processing loop would then be broken with a view to getting more data. Although...

...it does look like the write pointer isn't updated when the callback returns 0!