What’s wrong with this code, part 7: The answers

To fix the newsgator issues, I switched to using blogjet to post to my blog.  Unfortunately, apparently there are some issues with my workflow that need to be resolved, this was posted yesterday, but didn’t get noticed on most peoples aggregators (I’ve not yet seen the post in newsgator, for example).  So I’m reposting it to let people pick it up as a new post.  I’m still trying to figure out how to get past the double encoding problem…  Sorry about the duplicate post 🙁

As I mentioned in yesterday’s post, there are two intentional bugs in the code.

The first bug is a huge one.  Remember my comment about the context of the API: “a service author decided that…”.  The problem is that the HKEY_CLASSES_ROOT predefined registry key they used to open the classes database cannot be used in a service (or in an application that impersonates users). 

The problem is that the HKEY_CLASSES_ROOT key isn’t a real registry key – it contains registry entries from both HKEY_LOCAL_MACHINE and HKEY_CURRENT_USER.  To retrieve the values from HKEY_CURRENT_USER, advapi32 opens this key under the covers and caches the result until the process is terminated.

And opening HKEY_CURRENT_USER is a big no-no when you’re running as a service.  The problem is that the registry handle for HKEY_CURRENT_USER (or any of the user hives) keeps a reference to the users token.  For the NT authentication scheme this extra token isn’t horrendously bad (although it WILL cause major problems with romaing profiles, and thus should be avoided at all costs).  But there are other authentication mechanisms available for Windows that require that a user only be logged onto a single machine at a time.  This means that this token handle that’s being kept open by your service prevents that user from logging onto ANY machine on the network, and will continue to prevent the user from logging on until your service terminates.

To fix this problem, as the documentation for HKEY_CLASSES_ROOT states, you should NEVER use HKEY_CLASSES_ROOT from a service (or from an interactive application that impersonates users other than the logged on user).  Instead, you should use RegOpenUserClassesRoot to open the per-user class database, and HKEY_LOCAL_MACHINE\Software\Classes when opening the per-machine class database.

Btw, all of the COM APIs know about this restriction, and they correctly respect the HKLM vs HKCU split.  So if you’re using the documented COM APIs, you won’t ever have a problem.

And that leads to the second bug in the code.  The second bug is somewhat more hypothetical, but no more significant.  Essentially, the structure of the data under HKEY_CLASSES_ROOT is considered to be internal-only.  The COM registry entries are fully documented to allow a user to add a COM component, but they do not guarantee that you can determine which DLL is going to be loaded by following the rules laid out in my “How does COM activation work anyway” post.  If a COM object is distributed as a side-by-side assembly then the rather simplistic rules I wrote about in my “How does COM activation work anyway?” post for InprocServer32 activation don’t completely describe the process by which the DLL is loaded.

If you use CoCreateInstance, then the right thing will happen, but if you attempt to assume that you know what COM is going to do to activate your code, then at some point in the future, you’re likely to discover that you got it wrong and have to hotfix your code to resolve the issue.

So there are two takeaways for this “What’s wrong with this code”.  The first is “Don’t use HKCR unless you KNOW that your code will NEVER be invoked in a service”.  The second one is “The COM APIs are your friend, trust COM, it knows what it’s doing, do not presume that you can do a better job than it does, for therein lie dragons”.

Now for kudos and mea culpas:

I’m not surprised nobody found the second bug – it was an incredibly subtle bug, and not at all clear.

Once again, Mike Dimmick nailed the first bug.  In addition, he pointed out one of the major problems with HRESULTs – people use SUCCEDED to check for success, when S_FALSE is a success return code that means “I didn’t do what you wanted me to, but it wasn’t your fault” – S_FALSE is effectively the same as a failure lurking under the success class of return values.

Paul Winwood pointed out that I should have used a tighter restriction than KEY_READ in my security flags.

Peter da Silva and Mo pointed out that if the code was used in its stated purpose that there was a race condition that would cause registry entries to be doubly written.

Mike R asked if CoTaskMemFree should be SysFreeString.  The answer is NO, since StringFromCLSID returns a LPWSTR not a BSTR – SysFreeString can only be called on a BSTR – they’re different (see this most excellent post by Eric Lippert for details).

Simon Cooke pointed out that I could safe a registry round trip.  He also brought up issues with Win9x (which is usually outside the scope).

Chui Tey pointed out that RegOpenKeyEx might change the value of its out values in the failure case.  That’s possible, but HIGHLY unlikely, simply because the possibility of breaking applications. 

Pavel Lebedinsky pointed out (correctly) that if the service isn’t impersonating anyone that using HKCR is safe.  This is true on its face, but it needs to be made ABSOLUTELY clear.  There’s no enforcement of this, so…  And given the potential of people misusing HKCR, it’s safer to just ignore it.

Edit: I think this didn’t show up on peoples aggregators, so…

Comments (2)

  1. Mike Dunn says:

    Just FYI, both copies of this article showed up fine in SharpReader.

  2. claudia says:

    i don’t know what a zip code is do you know what it is please tell me please what is my zip code