What's wrong with this code, part 16

This real-world problem shows up as an appcompat problem about every two or three weeks, so I figured I'd write it up as a "What's wrong with this code" snippet.

 

BOOL DllMain(HMODULE hModule, ULONG Reason, PCONTEXT pContext){    ghInst = (HINSTANCE) hModule;    if (Reason == DLL_PROCESS_ATTACH) {        DWORD cWaveDevices = 0;        DisableThreadLibraryCalls();        cWaveDevices = waveOutGetNumDevs();        if ((cWaveDevices & 0xffff0000) != 0)        {            dprintf("Error retrieving wave device count\n");        }        < Do some other initialization stuff >    } else if (Reason == DLL_PROCESS_DETACH) {        < Do some other initialization stuff >    }     return TRUE;}

This one's really simple, and should be blindingly obvious, but it's surprising the number of times people get it wrong.

Comments

• Anonymous
  December 05, 2005
  I suppose waveOutGetNumDevs will trigger the loading of another dll... from DllMain. :D
• Anonymous
  December 05, 2005
  Never, ever, ever, never, never, ever do anything that will result in calls to external dlls (except kernel32.dll which is always loaded first and at the same location) from within DllMain. You're just asking for a heart attack 3 months after your code has shipped.
• Anonymous
  December 05, 2005
  I think that waveOutGetNumDevs() will return 0 on either an error condition, or when no devices are available. The code is instead checking for a different range of error codes.
• Anonymous
  December 05, 2005
  Calling DisableThreadLibraryCalls and expecting a DLL_PROCESS_DETACH appears to be wrong according to MSDN.
• Anonymous
  December 05, 2005
  The comment has been removed
• Anonymous
  December 05, 2005

1. You are not supposed to call functions other than those in KERNEL32 in DllMain. waveOutGetNumDevs could trigger other DLLs to be loaded triggering problems with the Loader Lock - potentially creating deadlocks and circular dependencies?
   2) Possibly the calling convention is missing WINAPI ?
   3) As documented the first parameter to DllMain is a HINSTANCE so the cast is unneccessary and the third parameter is LPVOID not a PCONTEXT .

• Anonymous
  December 05, 2005
  My 2 cents:
  
  Why is he checking for upper 16 bits? is he expecting more than 65535 waveOut devices?
  
  -Gopal
• Anonymous
  December 05, 2005
  The comment has been removed
• Anonymous
  December 05, 2005
  DisableThreadLibraryCalls will disable DLL_THREAD_ATTACH and DLL_THREAD_DETACH notifications for the DLL, so it should not affect the DLL_PROCESS_DETACH.
  
  Doing initialization stuff when a process is detached is not very logical.
  
  The ((cWaveDevices & 0xffff0000) != 0) part does not checks with the waveOutGetNumDevs definition in MSDN. If should be (cWaveDevices == 0).
  
• Anonymous
  December 05, 2005
  If you call DisableThreadLibraryCalls(), then you can forget about initializing the TLS index for any attaching thread.
• Anonymous
  December 05, 2005
  The number of wave devices may change at runtime. I hope the program isn't doing some initialization that depends on that amount staying the same.
  
  Dave, actually, I think you're also allowed to call functions in ntdll.dll and the primary Win32 client libraries (kernel32.dll, gdi32.dll, user32.dll and advapi32.dll)
• Anonymous
  December 05, 2005
  Hmmm, so the last parameter is really a PCONTEXT? Interesting...
• Anonymous
  December 05, 2005
  Actually the last parameter is LPVOID pvoid - a PCONTEXT is the same type as an LPVOID, I have no idea why the variable has that name.
  
  You test the last parameter for NULL vs non-NULL in DLL_PROCESS_ATTACH - if it's one way, it means you were unloaded via FreeLibrary, the other way means you were called because the process is exiting. This IS documented, btw.
  
• Anonymous
  December 05, 2005
  LoadLibrary is a kernel32.dll function. Can I call it safely from DllMain ?
  Is it the load of a library inside DllMain which breaks things or something else ? And, if it's ok, what if the DLL is a managed class library ?
  
  
• Anonymous
  December 06, 2005
  The last parameter is really a pointer to a context record. During LDR initialization (when snaps and DLL initialization are done for static linked images), the last parameter is a pointer to the context record restored after the initial user APC queued to the thread finishes. This context record describes the context that will be restored as soon as process initialization finishes.
  
  I suppose this is included as a parameter because any changes to the threads context made during early process/thread initialization would be wiped away after the NtContinue() call when LDR finishes initialization. For instance, this is why if you try to set a hardware (DR-based) breakpoint on x86 during process initialization, WinDbg gives you a warning that your breakpoint will disappear before the program's main function runs. By modifying the given context record you could persist, say, a hardware breakpoint after the initial DLL initialization finishes.
  
  (I know of nothing that actually uses this, though, and to my knowledge there is no documentation of it other than one or two samples from Microsoft that have slipped through and still use that parameter type).
  
  As for what's wrong with this code:
  
  - It's dangerous to try and call functions from DllMain that reside in DLLs that your DLL is dependant on. The reason is that in some cases that DLL may not have run its DllMain yet, and if those functions rely on some state initialized during DllMain they may not operate as expected. This situation doesn't occur always, and as far as I know, with the current implementation, you'll only really see it in the case of either circular dependencies or some situations where you are loading a DLL dynamically (i.e. not at process-time initialization). The exceptions to this are DLLs that have only depedencies that you are logically guaranteed to have already initialized by the time your DllMain occurs. These include NTDLL (always initializes first) and KERNEL32 for Win32 programs (only depends on NTDLL and the main process image or one of its dependencies will always have linked to it).
  
  - Depending on what dprintf does, that this might be bad. Particularly if it does something like make a CRT call and you are using the DLL version of the CRT. I would expect that it probably uses _vsnprintf or something of that sort, so I would flag it as dangerous.
  
  - Masking off the top 16-bits for waveOutGetNumDevs is curious; there should not be any reason to do this according to the documentation for that function. Perhaps this is an artifact of some code ported from Win16, or maybe there is some implementation detail of waveOutGetNumDevs that this DLL is relying on.