What's wrong with this code, part 16, the answers

  • Article

I intentionally made the last "What's wrong with this code" simple, because it was just intended to exhibit a broken design pattern.

The bug appears here:

        cWaveDevices = waveOutGetNumDevs(); 

The problem is that waveOutGetNumDevs is in winmm.dll, and MSDN clearly states:

The entry-point function should perform only simple initialization or termination tasks. It must not call the LoadLibrary or LoadLibraryEx function (or a function that calls these functions), because this may create dependency loops in the DLL load order. This can result in a DLL being used before the system has executed its initialization code. Similarly, the entry-point function must not call the FreeLibrary function (or a function that calls FreeLibrary) during process termination, because this can result in a DLL being used after the system has executed its termination code..

Because Kernel32.dll is guaranteed to be loaded in the process address space when the entry-point function is called, calling functions in Kernel32.dll does not result in the DLL being used before its initialization code has been executed. Therefore, the entry-point function can call functions in Kernel32.dll that do not load other DLLs. For example, DllMain can create synchronization objects such as critical sections and mutexes, and use TLS.

Calling functions that require DLLs other than Kernel32.dll may result in problems that are difficult to diagnose. For example, calling User, Shell, and COM functions can cause access violation errors, because some functions load other system components. Conversely, calling functions such as these during termination can cause access violation errors because the corresponding component may already have been unloaded or uninitialized.

The problem with waveOutGetNumDevs() (and all the other MME APIs) is that under the covers they call LoadLibrary (to load wdmaud.drv).  They also read from the registry, RPC into the audiosrv service, and lots of other stuff.  In Windows XP, it appears that a fair number of applications got lucky and didn't hit the deadlocks inherent in these functions, but for Vista, one of the consequences of the new audio engine architecture is that it is now 100% guaranteed that if you call the MME APIs from your DllMain, you are absolutely going to deadlock your application.

 

Kudos:

The first poster: Mike asked exactly the right question - waveOutGetNumDevs will trigger the loading of another DLL, as did most of the other commenters.

Seth McCarus thought it was "interesting" that the last parameter was of type PCONTEXT. Since a PCONTEXT is a PVOID, it's not really that surprising.

Skywing made the most complete description of the errors, I'm including it entirely because it's well written (I edited out the APC stuff because (to be frank) I have no idea if that's true or not):

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.

Mea Culpas:

My code was wrong - the internal message that's used to get the number of devices returns an error code in the high 16 bits of the returned count of devices, for some reason, I assumed that error propagation mechanism applied to waveOutGetNumDevs, it doesn't.