What's wrong with this code, part 15

Artikel
10/28/2005

Work's been pretty hectic lately, that's why so few posts this month, but I ran into a real bug in my code recently that I realized would make a GREAT "What's wrong with this code" post.

 HRESULT CNotification::GenerateEvent 
   (     
      PNOTIFICATION_BLOCK NotificationBlock
   ) 
{ 
    HRESULT hr = S_OK; 
    BYTE *buffer = NULL; 
    DWORD bufferSize = sizeof(NOTIFICATION_HEADER) + 
                            NotificationBlock->BlockSize; 
    if (NotificationBlock->BlockSize < sizeof(NOTIFICATION_BLOCK)) 
    { 
        hr = E_INVALIDARG; 
        goto Error; 
    } 
    else 
    { 
        buffer = new BYTE[bufferSize]; 
        if (buffer == NULL) 
        { 
            hr = E_OUTOFMEMORY; 
            goto Error; 
        } 
        PNOTIFICATION_HEADER notificationHeader = (PNOTIFICATION_HEADER)buffer; 
        PNOTIFICATION_BLOCK notificationBuffer; 

        ZeroMemory(buffer, bufferSize); 
        notificationBuffer = (PNOTIFICATION_BLOCK)(notificationHeader + 1); 

        <Fill in the EVENT_TRACE_HEADER within the NOTIFICATION_HEADER structure>

        CopyMemory(notificationBuffer, NotificationBlock, NotificationBlock->BlockSize); 

        hr = HRESULT_FROM_WIN32(TraceEvent(_TraceHandle, (PEVENT_TRACE_HEADER)&notificationHeader._TraceHeader));
        if (hr != S_OK) 
        { 
            goto Error; 
        } 
    } 
Cleanup: 
    delete []buffer; 
    return hr; 

Error: 
    goto Cleanup; 
}

Pretty straightforward, but it's got a REALLY nasty bug in it (I was SO embarrassed when I found it).

As always, kudos and mea culpas next post.

Edit1: Fixed typo (mediaBuffer->buffer). Also fixed NOTIFICATIONBLOCK that should be PNOTIFICATIONBLOCK

Comments

Anonymous
October 28, 2005
First thing that jumps out at me is that, depending on which RT you link against, new may throw.

Also, should your delete[] mediaBuffer be delete[] buffer?
Anonymous
October 28, 2005
And also, you don't check the buffer for validity before you delete
Anonymous
October 28, 2005
BlockSize != sizeof() vs Blocksize < sizeof()? That the first one I see.
Anonymous
October 28, 2005
If NotificationBlock->BlockSize is large, then sizeof(NOTIFICATION_HEADER) + NotificationBlock->BlockSize will overflow a DWORD, and you'll get a buffer overrun doing the CopyMemory.
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
You mean aside from the fact that you're assigning new BYTE[..] to buffer, and then deleting mediaBuffer?
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
the buffer is named buffer:
BYTE *buffer = NULL;

but Cleanup deletes a different variable:
delete []mediaBuffer;

so the function leaks memory, and some global variable(?) mediaBuffer is deleted...
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
Should I start with the fact that you dereference a parameter before checking whether or not it is null?
Anonymous
October 28, 2005
You're not checking if NotificationBlock is NULL.
Anonymous
October 28, 2005
I use the nonthrowing new, not the throwing new. And the delete mediaBuffer is a typo, fixed.

And I assume that if you pass a null parameter, we'll crash. That is very intentional.
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
Actually looked hard this time. You can't have an event structure larger than 64k according to MSDN. There's no check for this. Not sure what the repercussions are, but that's something.
Anonymous
October 28, 2005
It's got something to do with

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

Something like HRESULT_FROM_WIN32 assuming that the return from TraceEvent is always 32bit.

That me out on a limb.
Anonymous
October 28, 2005
Hmmm...so what happens if NotificationBlock is NULL??

I guess thats not it because it's just too obvious.
Anonymous
October 28, 2005
Actually scratch both my previous commentsm they're off the mark.

But surely it must revolve around this statement regarding HRESULT_FROM_WIN32 the MSDN lib:

"Maps a WIN32 error value into an HRESULT. Note that this assumes WIN32 errors fall in the range of -32k to 32k."
Anonymous
October 28, 2005
Got it, it's a simple RTM problem.

TraceEvent takes a handle and a pointer to an event object.

You are passing a handle (I assume) and a pointer to a pointer = a random

replace:
TraceEvent(_TraceHandle, (PEVENT_TRACE_HEADER)&notificationHeader._TraceHeader)

with:
TraceEvent(_TraceHandle, (PEVENT_TRACE_HEADER)notificationHeader._TraceHeader)
Anonymous
October 28, 2005
hr != S_OK??

The HRESULT_FROM_WIN32 macro shouldn't set a success HRESULT but it's not good practice.

Initially assuming success (hr = S_OK) isn't really good practice either.

You can only pass a maximum of 64KB of user data to the TraceEvent function; if you send more data than that you'll get an ERROR_MORE_DATA error returned. MSDN isn't clear on whether the data that would fit has been traced or not.

Otherwise I can't see the issue.
Anonymous
October 28, 2005
Not sure why my previous comment does not appear so here it is again:
HRESULT_FROM_WIN32 evaluates its argument many times. TraceEvent will be called multiple times. Morale: Never put function call inside a macro one.
Anonymous
October 28, 2005
HRESULT_FROM_WIN32(x) macro executes x twice, once to check the result's failure bit, twice to compose the final HRESULT. The event will be traced twice.
Anonymous
October 28, 2005
notificationBuffer = (PNOTIFICATION_BLOCK)(notificationHeader + 1);

should be
notificationBuffer = (PNOTIFICATION_BLOCK)(notificationHeader + sizeof(NOTIFICATION_HEADER));
Anonymous
October 28, 2005
Assume NotificationBlock->BlockSize == 0xFFFFFFFF.

The guard if performs unsigned comparison and won't be triggered.

CopyMemory will overwrite the whole memory (well, a considerable part of it).

Suggestions:

1) perform signed comparision on NotificationBlock->BlockSize;

2) it doesn't hurt to check that NotificationBlock is not NULL
Anonymous
October 28, 2005
wait, notificationHeader is a PNOTIFICATION_HEADER so that might work. It would be kind of hard to miss if it wasn't working.

But
notificationBuffer = (PNOTIFICATION_BLOCK)(buffer + sizeof(NOTIFICATION_HEADER));

should work, right? buffer is a BYTE* so that pointer arithmetic should be safe...
Anonymous
October 28, 2005
HRESULT_FROM_WIN32 will call TraceEvent twice.
Anonymous
October 28, 2005
hr = HRESULT_FROM_WIN32(TraceEvent(_TraceHandle, (PEVENT_TRACE_HEADER)&notificationHeader._TraceHeader));

typo? &notificationHeader->_TraceHeader
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
There are several HRESULT_FROM_WIN32 macros defined in VC++ 2003, one of which is:

((HRESULT)(x) <= 0 ? ((HRESULT)(x)) : ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)))

Using this macro would mean TraceEvent is called multiple times.
Anonymous
October 28, 2005
The comment has been removed
Anonymous
October 28, 2005
A small bit of advice for MS (and others)

inline HRESULT HResultFromWin32(DWORD err)
{ return HRESULT_FROM_WIN32(err); }

and similar for SUCCEEDED and FAILED. Yes I know these two are "safe" but to rely on this you have to rely on implementation details. Don't follow bad examples from MSDN that love to have

if (SUCCEEDED(func()))
and
if (FAILED(func()))

everywhere. Inline functions have been in C++ for a long time and there is no excuse for a code sprinkled with CSomething not to use them.
Macro "functions" (as opposed to code generation and conditional compilation macros) are evil
Anonymous
October 28, 2005
I agree with Maurits. You're going to all the trouble of creating a buffer large enough for the NOTIFICATION_HEADER and the notification block, creating the header in the right place and then overwriting it by copying the notification block into the buffer at an offset of 1, instead of after the header. I can only assume this is a typo in the question because otherwise your trace block would be gibberish.
Anonymous
October 29, 2005
Kudos to Marvin. :)

But the pointer arithmetic thing still concerns me.

Suppose for the sake of argument...
A NOTIFICATION_HEADER is seven bytes long
You're compiling a 64-bit binary

Isn't it possible that the compiler would want to lay out a NOTIFICATION_HEADER[] on word boundaries? It only wastes a single byte per array element...

In which case, isn't the (notificationHeader + 1) dangerous? You might be a byte off in the memory copy, leading to
* use of uninitialized data (CopyMemory starts in the wrong place)
* a buffer overflow (CopyMemory writes its last byte to unallocated memory)

On the other hand, char-pointer-arithmetic is explicitly safe by the C spec. So (assuming BYTE is typedef'd to char) the (buffer + sizeof(NOTIFICATION_HEADER)) offset should be right under all wordsizes.
Anonymous
October 29, 2005
C++ Standard: 5.3.3.2

When applied to a class, the result is the number of bytes in an object of that class including any padding required for placing objects of that type in an array.

If this wasn't true, then sizeof (x) * 2 might not equal to sizeof (x[2]). Larry's code is fine.

The real issue that can be of concern is not the different between sizeof(x) or + 1, but with the alignment of the object following it. If you aren't careful with how you construct the first object, then if the second object has different alignment requirements (lets say a struct of a single LONG followed by a struct of a single double), the second structure might be misaligned.
Anonymous
October 29, 2005
The comment has been removed
Anonymous
October 31, 2005
HRESULT_FROM_WIN32(GetLastError()) occurs 34 times in ATLCOM.H and STATREG.H.
Anonymous
October 31, 2005
GetLastError() is not a modifying operation, so it doesn't matter (except being redundant calls, of course). TraceEvent has side effects.

Freigeben über

What's wrong with this code, part 15

Comments

Zusätzliche Ressourcen