Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 5

What's wrong with this code, part 5

  • Comments 53

So there was a reason behind yesterday’s post about TransmitFile, it was a lead-in for “What’s wrong with this code, part 5”.

Consider the following rather naïve implementation of TransmitFile (clearly this isn’t the real implementation; it’s just something I cooked up on the spot):

bool TransmitTcpBuffer(SOCKET socket, BYTE *buffer, DWORD bufferSize)
{
      DWORD bytesWritten = 0;
      do
      {
            DWORD bytesThisWrite = 0;
            if (!WriteFile((HANDLE)socket, &buffer[bytesWritten], bufferSize - bytesWritten, &bytesThisWrite, NULL))
            {
                  return false;
            }
            bytesWritten += bytesThisWrite;
      } while (bytesWritten < bufferSize);
      return true;
}

bool MyTransmitFile(SOCKET socket, HANDLE fileHandle, BYTE *bufferBefore, DWORD bufferBeforeSize, BYTE *bufferAfter, DWORD bufferAfterSize)
{
      DWORD bytesRead;
      BYTE  fileBuffer[4096];

      if (!TransmitTcpBuffer(socket, bufferBefore, bufferBeforeSize))
      {
            return false;
      }

      do
      {
            if (!ReadFile(fileHandle, (LPVOID)fileBuffer, sizeof(fileBuffer), &bytesRead, NULL))
            {
                  return false;
            }
            if (!TransmitTcpBuffer(socket, fileBuffer, bytesRead))
            {
                  return false;
            }

      } while (bytesRead == sizeof(fileBuffer));

      if (!TransmitTcpBuffer(socket, bufferAfter, bufferAfterSize))
      {
            return false;
      }
      return true;
}

Nothing particular exciting, but it’s got a big bug in it.  Assume that the file in question is opened for sequential I/O, that the file pointer is positioned correctly, and that the socket is open and bound before the API is called.  The API doesn’t close the socket or file handle on failure; it’s the responsibility of the caller to do so (closing the handles would be a layering violation).  The code relies on the fact that on Win32 (and *nix) a socket is just a relabeled file handle.

As usual, answers and kudos tomorrow.

 

 

 

  • Can ReadFile hand you back less than sizeof(fileBuffer) bytes even if its not at the end?
  • Nicholas: Good thought, but not if it's reading from a file.

    But that's a good point: Assume that the file handle's connected to a file, not a named pipe.

    This one's REALLY subtle, and a hint may be needed.
  • Well, I'm not a big fan of the do while() in TransmitTcpBuffer; if buffersize is zero, a WriteFile is still performed. Not specifically against the rules according to the doco for WriteFile, but still a bit niggling.

    WriteFile should wait until it has transmitted the entire contents of the buffer before returning, even if it can't be transmitted in one lump, as written (that is, if I recall the semantics for blocking IO on WriteFile calls correctly - they're not spelled out in the docs). So the code in TransferTcpBuffer may be overkill, but should work fine.

    One API design bug might be that MyTransmitFile doesn't specify how much of the file should be transmitted; at the moment it transmits everything until it hits an EOF condition.

    Other than that, I'm stuck for now.
  • There's no general contract for what WriteFile will do on a null write but sockets specifically define a behavior.
  • Simon, You're right, MyTransmitFile should take a length, but that would have complicated the code, and introduced the potential for unintended bugs (I'm wary of unintended bugs after the first "what's wrong with this code fiasco (http://weblogs.asp.net/larryosterman/archive/2004/04/27/121299.aspx).

    As I said in my first comment, this one is subtle (but it's a surprisingly common problem).
  • The value of

    DWORD bytesThisWrite = 0;

    Goes out of scope all the time, and is thus resetted, hence a very long loop in case of a write where the socket couldn't do a full complete write.

    Would this be the one you are looking for?
  • Bad me, read the code wrong.. The names are too equal =) That was not the bug
  • So, didn't Niclas get it right? If the file is exactly a multiple of 4096 bytes in length, a WriteFile of 0 bytes will be performed. This triggers a shutdown at the TCP level, such that the following write of bufferAfter will fail.

    I can't see anything else wrong anyway :).
  • 1. There is no validation for null pointers.
    2. Socket may be put in non-blocking state, I don't know what'll ReadFile and WriteFile do in that case. Handle WSAEWOULDBLOCK error!

    That's all I could find.
  • Stephane, does it? I thought that TCP swallowed a 0 byte write.

    "I can't see anything else wrong anyway :)."

    You're on the right track Stephane. I was serious about saying this was subtle, but surprisingly common. In fact, I was called in earlier this week to look at a version of this problem.

    Anonymous: Good catch! And that case should have been checked. That's the first unintentional bug.

  • Found my glasses and will try to read the code properly this time....

    Are we talking blocking or non blocking operations on the TCP socket?

    The code implies non blocking, but if so, we are missing alot of useful code, so I presume it is blocking mode?
  • Niclas, assume blocking. As you mentioned, non blocking means there needs to be a lot more code.

    Also assume that there's someone on the other end who'se reading the data (so the fact that blocking sockets can hang if the guy on the other end hangs) isn't a problem.
  • Didn't you mention in your previous post one has to lock the socket to prevent other threads from writing to it before the transmit is complete?
  • That's the real TransmitFile, not my toy implementation. Good thought though :)
  • One more try - network byte order vs. host byte order?
Page 1 of 4 (53 items) 1234