Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 7

What's wrong with this code, part 7

  • Comments 22

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 :)

 

  • 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_ROOT\CLSID 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).
  • I suspect there's a race condition if two services using this technique start concurrently.
  • Rather than KEY_READ would KEY_ENUMERATE_SUB_KEYS be more suitable for the first RegOpenKeyExW? Plus you don't need change notification permission.
  • Paul, you're right, for this purpose, KEY_ENUMERATE_SUB_KEYS might be more appropriate. I just threw the example together, and I knew that KEY_READ would work.

    Peter, I'm not aware of any race conditions, could you go into more details?
  • The race condition could occur if two instances of the same executable are invoked. Not common, but possibly. The negative effects are probably not catastrophic.

    I'm not sure precisely what's wrong with the code (ignoring the 'goto'), except that poking around in the registry is the job of the COM APIs, not your application directly. Though, how does this code get invoked in the first place?

    If I'm reading correctly, this code is in a client-side library which interfaces via COM with a server program that runs as a service. If this is the case, there are several problems; more on those in a moment.

    If it's a plain old application, this is quite bad; the application is installed, but the classes aren't available for use until it's first executed - this breaks OLE (and, of course, DCOM).

    If it's an in-proc server, the problems are more than a little obvious.

    Why would I consider the first case problematic?

    First - registration should be performed by the executable which hosts the classes. You can run into serious sync problems otherwise.

    Secondly - the effort of doing this outweighs the benefits of delayed registrations. Self-registration isn't difficult.

    Thirdly - In the DCOM scenario, this fails miserably. The executable that hosts the classes is on a different host to the one which implements the APIs (and hence checks for registration). Everything falls apart.

    That is, assuming I'm reading the scenario correctly.
  • What happens if StringFromCLSID returns a success code that isn't S_OK? You should always test the SUCCEEDED macro unless you have a reason to test for other cases (some APIs return S_FALSE to mean something different from S_OK, for example).

    Race conditions: presumably if two instances run concurrently, they will both detect that the class isn't registered, and both register it. That shouldn't be a problem, though, as they will be writing the same information.

    The next issue to note is that this registration code will run as the user running the program. HKEY_CLASSES_ROOT in Windows 2000 and higher is a merged view of two keys: HKEY_LOCAL_MACHINE\Software\Classes and HKEY_CURRENT_USER\Software\Classes. This can have interesting implications. If the key already exists in HKEY_CURRENT_USER, and you write to HKCR, the system directs it to HKCU. Otherwise it's directed to HKLM. Only administrators have rights to write to HKLM, so an update may fail if the user's not an administrator. Alternatively the change may only take effect for the current user, not all users on the system.

    Another thing to consider is that the user's profile may not be loaded. CreateProcessAsUser does not load the user's profile. I can't recall if the SCM or COM SCM (different code; the first loads services while the second loads COM servers) load the user's profile or not. If the user's profile is not loaded, IIRC uses of HKEY_CURRENT_USER map to HKEY_USERS\.DEFAULT.

    The current recommendation is not to write self-registration code at all (or not rely on it for deployments). Instead, you should use Windows Installer to install your application and specify the keys explicitly. This allows Windows Installer to repair incorrectly modified registry keys on launching the application.

    In general things need to be improved with regard to file associations and other HKEY_CLASSES_ROOT keys. I'm still annoyed that I can't repair my file associations with Windows Media Player when I'm not an administrator - the File Types tab doesn't appear, even in v10. See http://mikedimmick.blogspot.com/2004/01/real-need-good-kicking.html.

    The limited-user story is OK with my network administrator hat on, but horrible once I get home.
  • Ah, you're right. My bad - the problem isn't a single-lock-check pattern issue, but if the code was used in the scenario I mentioned above, there absolutely IS a race condition.

    But that's not the bug. The code as written is incorrect given the scenario.
  • Oh, and of course, as Jerry says - just because the key is there doesn't mean it's correct, or in fact in any way valid. The presence of HKCR\CLISD\guid alone is pretty meaningless.
  • 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).
  • 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).

  • Shouldn't CoTaskMemFree be SysFreeString?

    Also -- What is NO_ERROR... Is it the same thing as ERROR_SUCCESS?
  • Yup. Don't ask why there are two different error codes that mean "success".

  • Another stab -- Why not concatenate the class id to the path:

    e.g.CLSID\{guid}

    Of course, I'm being overly picky here, but this way you save a handle... :)
  • 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.
  • Personally I would not have assumed, if for example RegOpenKeyExW fails then classesKey is still NULL. That's what error codes are there for.
Page 1 of 2 (22 items) 12