Wishful Coding

Related to SuperstitiousCode, and WishfulThinking. The all-too-common pattern of writing code that can't possibly work, but which you'd like to work; a cause of nasty bugs if you're not using unit tests if the code appears to work and the wish causes you not to question the thinking.

I witnessed a truly horrible example just a few weeks ago (so this isn't an imaginary pathology or a historical one) in a smart person who's been programming for over 10 years (so don't go thinking you're immune, either), approximately as follows :

 CSocketDerived::send_buffer(CSession& session, const void* lpBuf) {
int nBufLen = sizeof(lpBuf);
int nFlags = 0;
CSocket::send(session.get_header(), HEADER_SIZE, nFlags);
CSocket::send(lpBuf, nBufLen, nFlags);
 }

The sizeof(lpBuf) returns probably 4, the size in bytes of a void* on most platforms. It might return 2 or 8 on other platforms, but it most certainly does not return the length of the buffer that lpBuf points to, which is what the programmer wished it did.

...and, in the instance I witnessed, unfortunately turned out to be the length of the buffer my colleague had passed to the function to test it. There's a sad, sad thing, in fact : experienced programmers do write unit tests in the sense that XP recommends. But then they throw them away. So he did that, and thought the silly thing was working, and when it stopped working - as soon as larger buffers started being sent around - he spent the better part of a day tracking it down.

There still may be deeper problems:

You don't care about the size of the buffer, you care about the amount of data in the buffer. The result is that you end up sending padded out packets which can cause problems at the receiver. Been there, seen that.
What happens when the data is over the OS's limit for a single send (typically 4096 bytes)? You have to loop the call to CSocket::send() until the buffer is completely sent. This should be handled in the base class. Which base class? CSocket. CSocket is an MFC thing, which doesn't handle that loop, if I'm Googling correctly.

Notice also the cleverness that has made CSession the actual object, and CSocketDerived a bucket of foreign methods to CSession.

Those are kind of besides the point, and probably even my own fault as I wrote the above snippet from memory. The one guaranteed true element to this story is about taking sizeof of a void pointer, expecting to get the size of a dynamically allocated buffer back.

Sorry for this uncharitable comment, but taking sizeof on that pointer is not an "oops!" mistake.

You're quite right. That was my whole point, in fact.

It is not even a mere coding error at the assignment statement. Designing an operation that acts on an untyped buffer without requiring a length to be somehow passed demonstrates a fundamental lack of understanding about the C language or programming in any low-level language.

Ah. In other words, you are saying - correct me if I misinterpret - that any person with a basic understanding of C programming would never, ever ever, make such a mistake ? Perhaps this begs the question of what a basic understanding of C is...

What language has this "a smart person who's been programming for over 10 years" been using all those years? The only forgivable source of such an error in C is a mindless cut-and-paste heist from a method that took a structure.

My own model is that it doesn't matter what language he's been using, and that we can make the assumption that he's reasonably competent by the usual standards. I want to explain such errors on purely psychological grounds. I'm aware that this is going to take a bit of work - I'm hoping you'll be willing to help me.

It's this sort of code that can easily lead to buffer overflows and various other nasty stack-bashing that many security problems you hear about take advantage of.

What language has this "a smart person who's been programming for over 10 years" been using all those years?

10 years experience of a language, can mean 10 repetitions of the same unthinking year, never gaining a deeper understanding than he had after one week. Unfortunately its very easy to hold down a job and progress with that level of competence.

Whoah! Give them a break!

I do agree that the code is fundamentally broken, as no size is being passed in. But 10 years of experience doesn't mean 10 years of socket programming.

I've written to Novell's API, NetBIOS, Sockets, WNet and heaven knows what else. You encapsulate pretty quickly, so simply don't spend much time at the raw level. And most people spend far less time doing network programming than I have.

One could, for instance, charitably assume that the person concerned:

hadn't done network programming for a while (if ever)
didn't have a great understanding of the area
didn't have anyone they could clear up questions with

I come across the attitude of errors indicating some kind of moral failure on the part of the programmer far too often for comfort. That's not a productive way to approach things. Better to learn the appropriate lessons to stop one repeating that error, and move on. In this person's case, that might include (a) refreshing their networking knowledge by reading a book on the subject, and (b) making it easier for them to ask other people for help if they're not sure what they're doing.

Of course, if they made analogous mistakes all the time, there's also (c) encourage them to find a different career.

I mostly agree, especially about that 'smarter than thou' attitude. I also think TestFirst will help a lot, as it makes WishfulCoding far less likely.

As a person who has made ... no, makes this kind of stupid mistake dozens of times, I consider myself an expert on it. ;-)

I could easily end up with the example code by the following steps:

start by writing a function that takes a struct (not a pointer)
later, realize that the type can't be complete at compile time, or would require rearranging half the class hierarchy to be less specific than void*, or maybe it just plain doesn't matter what the type is and a void* is easier
forget that the length is not passed as a parameter and can no longer be extracted from the argument type

Tcl is a beautiful (or horrible ;-) language for learning the kind of discipline that will protect you from this kind of mistake. Some Tcl functions act on variables by name instead of by value. Variable names like "4" and "" (that's the number 4 and an empty string) are actually valid in Tcl. I don't know anyone who hasn't learned the hard way about Tcl expressions like "lappend $foo $bar" (append the value of the variable named bar to the value of the value of the variable named foo, which is usually not what you want, especially if you were thinking in Perl while writing Tcl code.

--ZygoBlaxell

sizeof() of a pointer is a rookie mistake. It can also happen in a plain vanilla context, not necessarily a more complex area like sockets.

PierreCloutier