Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 15 - the answers

What's wrong with this code, part 15 - the answers

Rate This
  • Comments 15
Well, that was certainly interesting.  My last "what's wrong with this code" post had to do with a real-world bug in some of the notification infrastructure that's used for audio.  I took some sample code done while developing the prototype, and stripped out anything that would have described the actual infrastructure (NDA issues, sorry :)) to make the example.  As a result, except for typos (like one case where I missed some old code, and an incorrect typo), the code is exactly like I saw it.

As was mentioned in the comments , the actual bug was here:

        hr = HRESULT_FROM_WIN32(TraceEvent(_TraceHandle, (PEVENT_TRACE_HEADER)&notificationHeader._TraceHeader));
 

The problem is that the HRESULT_FROM_WIN32 macro is defined as:

#define HRESULT_FROM_WIN32(x) ((HRESULT)(x) <= 0 ? ((HRESULT)(x)) : \
                    ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)))
 

Unfortunately, that macro evaluates its argument twice - once when determining if the value is less than 0, the second time when forming the actual error code.

For some APIs without side effects (like GetLastError()), this isn't a big deal, but for APIs that DO have side effects (basically any API that returns a Win32 error code), it can be catastrophic.  The problem we had was relatively minor (we sent doubled notifications), but this pattern can easily result in memory and handle leaks.  Mike Dimmick pointed out that for Vista, this issue has been addressed, there is now an optional in-line version of HRESULT_FROM_WIN32 that doesn't have this problem, unfortunately, this code didn't use it :(

 

So Kudos and mea culpas:

First the mea culpas:  There were two typos in the original example - I left in some old code, and didn't get the type of one of my casts correct.

Kudos: Marvin was the first person who caught that HRESULT_FROM_WIN32 was a macro.  Close on his heals was K.T., Thales and Harold.

Now for other things that came up in the comments thread.

A number of people had concerns about the use of the "notificationBuffer = (PNOTIFICATION_BLOCK)(notificationHeader+1)" construct.  The format of the buffer in question has a common structure (a notification_header) at the head, and  a variable structure (the NOTIFICATION_BLOCK) following.  the "notificationHeader+1" returns an appropriately aligned structure located immediately after the NOTIFICATION_HEADER into the buffer.  Given that the block was allocated using sizeof(NOTIFICATION_HEADER), they correctly noticed that if NOTIFICATION_HEADER was misaligned, this could cause significant issues.  The +1 increments the notificationHeader by the number of bytes in a NOTIFICATION_HEADER, Tim Smith called out the line in the C++ standard where it shows that behavior is required.

A number of other people were confused about the NotificationBlock->BlockSize paradigm.  The idea is that the caller provides a self-describing buffer (NotificationBlock), and this routine packs it into the resulting block.  The NOTIFICATION_BLOCK provided by the caller had to be at least NOTIFICATION_BLOCK bytes long.   There IS an error here, because the caller doesn't provide the length of the block separately, I can't validate that the NotificationBlock->BlockSize is valid - I'm relying on the C++ compilers type safety to handle this - it may be wrong, but...

Once again, throwing new raised its ugly head, as did several people who wanted me to use exception handling as an error propagation mechanism, as did probing the NotificationBlock parameter for validity.  I've touched on this before - RAII has its own set of issues here, and I always use the non throwing new.  Oh, and someone was upset at the presence of a "goto" in the code.  Would it be better if I used a "do {} while (FALSE);" and then used "break" instead of the goto?  There's no semantic difference between the two.

There was also a potential issue of arithmetic overflow or underflow - since this function is internal-only, I'm not particularly worried about arithmetic overflow issues - if this was a public API, I would, but our team controls all the users of this function, so...

  • Best Practices for the alignment problem? Maybe make a NOTIFICATION struct with a fixed element for the header and a pointer for the notification block?
  • > "do {} while (FALSE);"

    Without actually compiling this, I think that construct will give a (in most cases benign) warning on level 4 - something about a constant boolean expression. I might remember wrong, though...
  • I would hope that compilers would special-case do{}while(false) and not issue a warning as long as there is a way to exit the block. Similarly, while(true){} and for(;;)
  • I never did that with HRESULT_FROM_WIN32(), but I once ASSERT()ed an assignment. Worked <em>just fine</em> in debug builds...
  • With VC++ (2003), both while(true){} and do{}while(false) (also do{}while(true)) give compiler warning "C4127: conditional expression is constant". for(;;){} doesn't give a warning.

    But still, apart from the warning, a do{}while(false) would be the same as the goto. Except that you wouldn't be able to nest another loop inside the do{}while(false) structure...
  • Yuck. Use a while loop when you need a loop. DON'T use it to avoid a goto as you WILL CONFUSE the developer that comes after you. The goto is by far the clearer choice as in fact the intent is to GO TO a different point in the code. Say what you mean.

    I strongly recommend reading "Gotos Considered Harmful and Other Programmers’ Taboos" at http://www.ppig.org/papers/12th-marshall.pdf. Unlearn what your professors taught you.
  • Presumably the original code was more complex, but I just don't see the need for a goto...

    Original code:

    if (buffer == NULL)
    {
    __hr = E_OUTOFMEMORY;
    __goto Error;
    }

    ...

    if (hr != S_OK)
    {
    __goto Error;
    }

    v2, no goto:

    if (buffer == NULL)
    {
    __hr = E_OUTOFMEMORY;
    __delete [] buffer;
    __return hr;
    }

    ...

    if (hr != S_OK)
    {
    __delete [] buffer;
    __return hr;
    }

    v3:
    if (buffer == NULL)
    {
    __return E_OUTOFMEMORY;
    }

    ...

    (note the entire second if() was deleted, since it didn't do anything different!)
  • Maurits, you're duplicating cleanup code. Right now, it's just a delete, but if someone were to later modify the function and add another resource that was acquired, they'd have to add the cleanup for that resource in multiple places. Having cleanup code in a single place for a function makes maintenance much easier, and given that code in an OS like Windows could have a lifespan of years, maintainability is incredibly important.
  • goto exception handling is just the C version of SEH (rather less structured), it's been argued over in this blog a few times before.

    "There was also a potential issue of arithmetic overflow or underflow - since this function is internal-only, I'm not particularly worried about arithmetic overflow issues - if this was a public API, I would, but our team controls all the users of this function, so..."

    Which brings to mind your cautionary tale about your 64-bit division function. I know, not the same thing since this is more specialized, but it made me smile. ;)
  • Complicating the epilog code in a method is an excellent way to one day inadvertantly miss an exit point, requiring Microsoft to spend hundreds of thousands of dollars identifying and repairing the defect.

    Using a goto is admittedly not very sexy, but it probably only costs a nickel of Larry's time per method to get things right.
  • Maurits, remember the problem I mentioned here: http://blogs.msdn.com/larryosterman/archive/2005/10/18/482399.aspx ?

    In that case, a developer who was helping our team out decided that for a function he was modifying it would be ok to return out of the middle of the routine. As a result of this, we leaked an impersonation (because I missed the codepath). Everything worked perfectly until many thousand instructions later when we called into an AAA function, at which point it went kabloom!

    It's far better to be totally consistant every time.
  • Larry, why don't you use RAII to wrap the buffer so it is automatically deallocated? (With boost::scoped_array for example, or by using a vector instead.) Then you don't need to use any gotos, you can just do "return S_INVALIDARG;" or something like that and the buffer is deallocated automatically.
  • OK, now it makes sense...

    So you have an idiom to the effect of...

    void foo(...)
    {

    void *p1 = allocate_resource1();
    void *p2 = allocate_resource2();

    do {
    __various(...);
    __things(...);

    __if (something->goes(HORRIBLY_WRONG))
    __{
    ____goto Error;
    __}

    __if (you_want[2]->exit(EARLY))
    __{
    ____goto Cleanup;
    __}

    } while (you_can());

    Cleanup:
    __free_resource1(p1);
    __free_resource2(p2);

    Error:
    __log(SOMETHING);
    __goto Cleanup;
    }

    In that case, I completely understand why there are goto's and labels in the original problem. Even if they're not needed for the particular function, there is value in consistency, and in future-proofing against memory leaks and other unintended changes.
  • And, of course, there should be a return; as the last part of the Cleanup: pseudocode
  • "but our team controls all the users of this function, so..."

    http://blogs.msdn.com/larryosterman/archive/2005/10/14/481137.aspx

    Never say never.

    - mn
Page 1 of 1 (15 items)