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."

Comments (16)
  1. Brian_EE says:

    Here is a blog entry where two different customers gave feedback about what they found the underlying issue to be. I'm curious what percentage of the time you receive that feedback vs not hearing anything back (which you've mentioned several times in the past).

  2. anonymouscommenter says:

    Isolation­"Aawre­"Create­Dialog­Indirect­ParamW ?

  3. @creaothceann – typo. It should be Isolation­AawRecreate­Dialog­Indirect­ParamW (a function to recreate cute indirect parameters for dialogs.)

  4. anonymouscommenter says:

    What do you use for grep?  Cygwin?  MingW tools?  Services for UNIX?  find.exe?

  5. anonymouscommenter says:

    I normally use the grep from GnuWin32 (though it is not downloadable until SourceForge stops being broken).

  6. Henri Hein says:

    For grep, I use unxutils.  I like its installation-free copy-and-run mode.  It has the added bonus of being an easy tag to get meaningful search results on.

  7. I'm going to bet that their MyClass::GetCurrentProcess returned some kind of actual pointer (say, a MyProcess*)…

    … which is then auto-converted to HANDLE without any compiler complaints, since HANDLE is a typedef for void*.

  8. anonymouscommenter says:

    Is there no shadowing warning in the compiler? If not, there should be.

    But IMHO the Real Cause of this one is that C++ adds an implicit "this->" when it sees something which could be a member. Other languages avoid this particular issue by requiring an explicit "this->" or "self." or similar.

    And Windows' namespace pollution doesn't help. It has thousands of global functions, many of them without any prefix or suffix; to have "a policy of not giving member functions the same name as global functions", you need to know all of their names, even the ones which will be created in the future (because there's no common prefix or suffix to avoid). I'd guess that the programmer who created the GetCurrentProcess method wasn't being cute; they just didn't know or didn't remember that there was a Win32 API function with that name (there are too many of them to remember). It's even worse for portable software: you just can't expect the Unix guys to know about all the many Win32 API functions.

    As for the bonus story, it wouldn't surprise me if the culprit had never heard of Isolation Awareness and all of its extra complexity, a lot of it hidden by redirecting macros and magic manifest identifiers.

  9. So did the customer not know how to use a debugger?

    [They knew how to use the debugger (that's how they found the problem), but they needed help with knowing where to look. -Raymond]
  10. anonymouscommenter says:

    Please fix the layout of your blog to be more mobile friendly.

    [Looks good on my phone. -Raymond]
  11. anonymouscommenter says:

    @looks like crap: You realise that your handle makes it less likely that anyone will bother, right? (I assume, by the way, that this is a temporary handle you've adopted for the purpose. Please excuse me if this is, in fact, your real name. You probably got teased a lot in the playground at school when you were younger and you don't need me having a go at you now.)

    For the curious, the right-hand margin appears to be set to about an inch, regardless of screen size, so the entire text (at least on my phone) is confined to a tiny strip down the left-hand side. The various boxes on the right are also missing, making the margin even more wrong.

  12. Chris J says:

    @Ryan – Powershell has grep in the form of select-string (aka sls), e.g. 'sls SELECT.*Merge" *.sql'

  13. anonymouscommenter says:

    @looks:  As Raymond has said many times here, he does not control the blog software.  He CANNOT fix things like that.  If you can't handle reading this blog on your device, I humbly suggest that you don't read it.

    Also, your handle probably violates the "be nice and respectful" policy.  :-)

  14. anonymouscommenter says:

    @ DWalker

    Agree with you.  I have never understood why people complain when someone else — out of pure generosity — gives them something for free.  Maybe it's just that I am officially an old-fart and see the value in being polite.  The goods news is that I have yet to yell, "Get off my lawn!" at kids in the neighborhood.

  15. anonymouscommenter says:

    @Chris J – Do you use Powershell as a programmer or in an IT capacity?  I have occasionally tried to do something with it and found it poor for coding types of problems, but excellent for "enumerate the network devices and change their policy" kind of problems.

  16. anonymouscommenter says:

    @Ryan – both. I've done log analysis scripts with it, replace batch files with it (as it's more predictable and less arcane than batch – for example, BCP files into a database based on certain criteria); validate configuration (including validating network endpoints stored in a database table by connecting to those endpoints and checking SSL), etc.

    In essence, where I might have written a Perl script or fired up VS to write a quick C# utility, Powershell replaces both and has the added benefit (over Perl) that it's pre-shipped on servers (which also means I don't need to persuade ops to put cygwin or unxutils on the box).

Comments are closed.

Skip to main content