Ayende @ Rahien

It's a girl

How NOT to write network code

It is surprising how many bombs you have waiting for you at a low enough level. Here is another piece of problematic code from the same codebase:

// Read the name.
var _buffer = new byte[_len];
int _numRead;
try {
    _numRead = stream.Read(_buffer, 0, _buffer.Length);
}
catch (Exception _ex) {
    logger.Log(string.Format("Conn: read(): {0}", _ex));
    throw new SpreadException(string.Format("read(): {0}", _ex));
}

// Check for not enough data.
if (_numRead != _len) {
    logger.Log("Conn: Connection closed during connect attempt");
    throw new SpreadException("Connection closed during connect attempt");
}

Can you spot the problem?

Comments

Tom Quinn
03/19/2009 05:28 PM by
Tom Quinn

_buffer.Length?

should be

_buffer.Length -1

Joel
03/19/2009 05:43 PM by
Joel

I think I see a couple issues:

  1. Catching a general Exception isn't a good idea, especially when working with buffers and I/O operations. An out of memory exception could occur and then you're trying to allocate more memory either in the logger.Log call or when creating the SpreadException.

  2. string.Format (called in two locations) could throw an exception that will not be caught, potentially creating a misleading error.

  3. logger.Log could throw an exception and result in a misleading error.

  4. The more interesting issue is:

If we assume that _len contains the length of the name to read off the wire, then the test _numRead != _len might create a misleading error. It is possible that reading from the socket won't return all the data available in the first call - for example, it might take two calls to Read(). It would be more appropriate to read the data off the wire in a loop and test whether the accumulated number of bytes read equals the expected length, or 0 (to indicated all data received/connection closed).

Also the exception doesn't make sense either. Firstly, the socket is likely already connected if you're trying to read from it. :) Furthermore, just because you didn't get all your data doesn't mean the connection has been closed. If the connection was aborted, you'd get an exception. If all the data had been read and the connection was closed, stream.Read() would return 0 to indicated a graceful disconnect.

Joel
03/19/2009 05:45 PM by
Joel

Tom,

No, you don't specify _buffer.Length - 1. You're not indexing the buffer, just specifying its length to the Read() function.

Peter Ibbotson
03/19/2009 05:54 PM by
Peter Ibbotson

Since this says network code, I'd guess that lots of the time

_len != _numread, and I'd be real unhappy about that exception calling itself "Connection closed"

Peter Ibbotson
03/19/2009 06:00 PM by
Peter Ibbotson

BTW Joel since you mentioned out of memory exceptions.

I noticed the other day that the raw Win32 api call WriteFile can throw ERRORNOTENOUGH_MEMORY if it has too many async IO calls running. I'm sure if this is because it calls WriteFileEx under the skin and I may have run into this in the wild with a third party library.

Anon
03/19/2009 06:18 PM by
Anon

numRead will only ever ==len if the received data was that size, but that is not always. It should be if if _numRead <= 0, but it also may not have been initialized.

Karl
03/19/2009 07:37 PM by
Karl

It's a pretty common to see networking code that expects stream reads to be perfectly aligned with stream writes (since that's largely how it will work when developing locally). In reality though, there is no such guarantee. Reading from a stream could return only a single byte - even though megs have been written on the other side. Similarly, multiple writes might be grouped into a single Read.

Developers need either using a StreamReader/StreamWriter which will block until the built-in delimiter is reached (I assume it's delimiter based), or write "smart" message. These messages normally have a header that says how long the message is (this can be tricky because the X bytes (say 4) you use for a header could itself be chunked into multiple reads)).

There's obviously an expectation here that 1 complete message will be read by each call to Read(). In reality it might take several calls to Read() - or, the single call might actually read multiple messages.

Oh, and the byte allocation could lead to pinning.

John Rayner
03/19/2009 08:12 PM by
John Rayner

From the Stream.Read MSDN docs, the return value "can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached.". Boom!

ralf
03/19/2009 08:32 PM by
ralf

I just had to google for the code and came across a company site that seems to belong to the author of the code you show.

I found this statement:

"[company's name] Selected by [rather big military contractor] for the U.S. Army's Future Combat Systems."

What has it been selected for? I hope just as a bad example.

Paulo K&#246;ch
03/19/2009 10:27 PM by
Paulo Köch

Reading as a C guy, @int _numRead;@ may never initialize. Comparing stuff without initializing is unpredictable.

Besides, the if should be insed the try block. If you failed to read, you don't check how many you read. You just failed, you've read nothing!

configurator
03/20/2009 12:10 AM by
configurator

@Paulo: If there was a chance _numRead were uninitialized, this wouldn't compile.

The only code path that can leave it uninitialized is the catch block, which has a throw in it so the if would never happen.

Omer Mor
03/20/2009 12:51 AM by
Omer Mor

A more subtle misuse of exceptions:

when writing:

try {}

catch (Exception ex)

{

//...

throw new SpreadException(...);

}

You should give ex as inner exception to the constructor of SpreadException.

Mario Pareja
03/20/2009 01:06 AM by
Mario Pareja

In addition to the several previously mentioned issues, we can't see when the stream is created, but I bet there isn't much in the way of disposing this resource in the event of an error.

Steve
03/20/2009 03:07 PM by
Steve

It does throw with an innerexception...

Craig
03/20/2009 08:20 PM by
Craig

if (_numRead != _len)

_numRead will return _len - 1. At least that is what was happening to me in some strangely similar code.

terminator
03/20/2009 08:38 PM by
terminator

buffer.Length? how to put it but some one has another choice to choose

theoffice
03/20/2009 08:42 PM by
theoffice

you don't specify _buffer.Length - 1. You're not indexing the buffer, just specifying its length to the Read() function.

theoffice
03/20/2009 08:42 PM by
theoffice

you don't specify buffer.Length and You're not indexing the buffer, just specifying its length to the Read() function.

Joel
03/22/2009 03:45 PM by
Joel

Steve,

Look closer, it actually doesn't throw with an inner exception specified. The inner exception is passed to the string.Format method and not the SpreadException constructor.

Comments have been closed on this topic.