July 22nd, 2015

When you think you found a problem with a function, make sure you’re actually calling the function, episode 2

A customer reported that the Duplicate­Handle function was failing with ERROR_INVALID_HANDLE even though the handle being passed to it seemed legitimate:

  // Create the handle here
  m_Event =
    ::CreateEvent(NULL, FALSE/*bManualReset*/,
                       FALSE/*bInitialState*/, NULL/*lpName*/));
  ... error checking removed ...


// Duplicate it here
HRESULT MyClass::CopyTheHandle(HANDLE *pEvent)
{
 HRESULT hr = S_OK;
 
 if (m_Event != NULL) {
  BOOL result = ::DuplicateHandle(
                GetCurrentProcess(),
                m_Event,
                GetCurrentProcess(),
                pEvent,
                0,
                FALSE,
                DUPLICATE_SAME_ACCESS
                );
  if (!result) {
    // always fails with ERROR_INVALID_HANDLE
    return HRESULT_FROM_WIN32(GetLastError());
  }
 } else {
  *pEvent = NULL;
 }
 
 return hr;
}

The handle in m_Event appears to be valid. It is non-null, and we can still set and reset it. But we can’t duplicate it.

Now, before claiming that a function doesn’t work, you should check what you’re passing to it and what it returns. The customer checked the m_Event parameter, but what about the other parameters? The function takes three handle parameters, after all, and they checked only one of them. According to the debugger, Duplicate­Handle was called with the parameters

hSourceProcessHandle  = 0x0aa15b80
hSourceHandle  = 0x00000ed8 m_Event, appears to be valid

hTargetProcessHandle  = 0x0aa15b80
lpTargetHandle  = 0x00b0d914
dwDesiredAccess  = 0x00000000
bInheritHandle  = 0x00000000
dwOptions  = 0x00000002

Upon sharing this information, the customer immediately saw the problem: The other two handle parameters come from the Get­Current­Process function, and that function was returning 0x0aa15b80 rather than the expected pseudo-handle (which is currently -1, but that is not contractual).

The customer explained that their My­Class has a method with the name Get­Current­Process, and it was that method which was being called rather than the Win32 function Get­Current­Process. They left off the leading :: and ended up calling the wrong Get­Current­Process.

By default, Visual Studio colors member functions and global functions the same, but you can change this in the Fonts and Colors options dialog. Under Show settings for, select Text Editor, and then under Display items you can customize the colors to use for various language elements. In particular, you can choose a special color for static and instance member functions.

Or, as a matter of style, you could have a policy of not giving member functions the same name as global functions. (This has the bonus benefit of reducing false positives when grepping.)

Bonus story: A different customer reported a problem with visual styles in the common tab control. After a few rounds of asking questions, coming up with theories, testing the theories, disproving the theories, the customer wrote back: “We figured out what was happening when we tried to step into the call to Create­Dialog­Indirect­ParamW. Someone else in our code base redefined all the dialog creation functions in an attempt to enforce a standard font on all of them, but in doing so, they effectively made our code no longer isolation aware, because in the overriding routines, they called Create­Dialog­Indirect­ParamW instead of Isolation­Aawre­Create­Dialog­Indirect­ParamW. Thanks for all the help, and apologies for the false alarm.”

Topics
Code

Author

Raymond has been involved in the evolution of Windows for more than 30 years. In 2003, he began a Web site known as The Old New Thing which has grown in popularity far beyond his wildest imagination, a development which still gives him the heebie-jeebies. The Web site spawned a book, coincidentally also titled The Old New Thing (Addison Wesley 2007). He occasionally appears on the Windows Dev Docs Twitter account to tell stories which convey no useful information.

0 comments

Discussion are closed.