What's wrong with this code, part 7

This rather fascinating issue came up in an internal DL on Friday.  Consider the following hypothetical code that return TRUE if a particular class is registered with COM:

bool IsClassRegistered(CLSID ClassID){ HKEY classesKey = NULL; HKEY clsidKey = NULL; LONG result; LPOLESTR classString = NULL; bool returnVal = false; result = RegOpenKeyExW(HKEY_CLASSES_ROOT, L"CLSID", 0, KEY_READ, &classesKey); if (result == NO_ERROR) { HRESULT hr; hr = StringFromCLSID(ClassID, &classString); if (hr != S_OK) { goto Cleanup; } result = RegOpenKeyExW(classesKey, classString, 0, KEY_READ, &clsidKey); if (result == NO_ERROR) { returnVal = true; } }Cleanup: if (classString != NULL) { CoTaskMemFree(classString); } if (clsidKey != NULL) { RegCloseKey(clsidKey); } if (classesKey != NULL) { RegCloseKey(classesKey); } return returnVal;}  

In this case, there are two known issues, one serious, one that is somewhat more hypothetical.  In order to understand both of these issues, it's necessary to understand the context for this code.  In this case, a service author decided that they wanted to be clever about their COM registration, and delayed their COM registration until the first user API called.  They wrote this function to see if they had already registered their class.

As always, answers tomorrow, with associated kudos and mea culpas :)

Comments

• Anonymous
  October 18, 2004
  I can't find anything wrong with the code per se (using nested if versus using goto to handle failures doesn't look good but would work as far as I can tell), but the logic seems to have problems - the fact that there's a key under HKEY_CLASSES_ROOTCLSID doesn't mean a class is registered and it doesn't mean it is the correct version (i.e. if they upgrade their product they'll never register the updated classes because the key will be there from previous install).
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  Rather than KEY_READ would KEY_ENUMERATE_SUB_KEYS be more suitable for the first RegOpenKeyExW? Plus you don't need change notification permission.
  
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  Bingo! Mike nailed the first of the two problems. HKCU can't be called from a service.
  
  Also, you're right for StringFromCLSID - the checks should be "hr != S_OK" for just that reason.
  
  Mike's 100% right too: Self registration code is potentially disasterous.
  
  There's still one more problem. To be honest, the second problem is subtle enough that I don't expect anyone who wasn't participating in the internal discussion to get it (but it's a really important problem issue).
  
• Anonymous
  October 18, 2004
  I should clarify; when I referred to 'self-registration', I did mean 'registering oneself when specifically told to - i.e., by an installer', though I'm given to understand that Windows Installer does all this for you now instead (apologies - I haven't worked with COM under Windows for several years, now).
  
  
• Anonymous
  October 18, 2004
  Shouldn't CoTaskMemFree be SysFreeString?
  
  Also -- What is NO_ERROR... Is it the same thing as ERROR_SUCCESS?
• Anonymous
  October 18, 2004
  Yup. Don't ask why there are two different error codes that mean "success".
  
  
• Anonymous
  October 18, 2004
  The comment has been removed
• Anonymous
  October 18, 2004
  OK.. really wild guess here...
  
  HKEY_CLASS_ROOT is probably a pretty widely used key, and potentially could be held open by many different parts of the system at once.
  
  There's a 65534 key open limit in the OS. So theoretically, it could prevent that key from being opened. Therefore it makes more sense to concatenate the string in our code and then pass it to the RegOpenKeyExW function, and let the registry code deal with the tree walking (which presumably it can do without hitting the limit).
  
  Of course, that's probably completely out there and not something that would happen in real life, but other than that I got nuthin'.
  
  
  
  Oh, wait a minute... how about this...
  
  If you're running on Windows 9x, StringFromCLSID will return an ANSI string not a Unicode string. RegOpenKeyExW expects Unicode strings only.
  
• Anonymous
  October 18, 2004
  Personally I would not have assumed, if for example RegOpenKeyExW fails then classesKey is still NULL. That's what error codes are there for.
• Anonymous
  October 18, 2004
  What's wrong with using HKEY_CLASSES_ROOT from a service if the code is not impersonating? It will probably be less efficient than going to HKLMSoftwareClasses directly, but otherwise I think it's fine.
• Anonymous
  October 18, 2004
  Mike: StringFromCLSID returns a string allocated with CoTaskMemAlloc (according to MSDN).
  
  PaulT: I'm pretty sure it's specified that the key will remain unchanged if the functions fail, but I could be wrong (I haven't checked).
  
  Pavel: Looking up is fine, actually performing the registration could well fail.
  
• Anonymous
  October 18, 2004
  Assuming return values of arguments I actually came across this a while ago with problem with LogonUser where the session handle would be changed if the logon failed so checking if the handle was null always failed.
• Anonymous
  October 19, 2004
  The documentation to RegOpenKeyEx states that:
  
  If the function succeeds, the return value is ERROR_SUCCESS.
  
  If the function fails, the return value is a nonzero error code defined in Winerror.h.
  
  There is no requirement that the clsidKey and classesKey be unchanged. This implementation detail can potentially change in a future version of Windows, therefore breaking the application.