You didn’t know you could add properties by atom, and it’s a good thing you didn’t know


As I noted a few days ago, there is weirdness associated with properties added by atom. This weirdness stems from the fact that adding properties by atom is really a hole in the original implementation rather than something designed on purpose.

The original 16-bit code for adding and removing properties went roughly like this:

BOOL SetProp(HWND hwnd, LPSTR pszName, HANDLE hValue)
{
    ... let's look only at the part that adds a new property ...

    ATOM atm = HIWORD(pszName) ? GlobalAddAtom(pszName) : LOWORD(pszName);
    if (atm == 0) return FALSE;

    ... add the atom "atm" to the property list ...
}

HANDLE RemoveProp(HWND hwnd, LPSTR pszName)
{
    ATOM atm = HIWORD(pszName) ? GlobalFindAtom(pszName) : LOWORD(pszName);
    if (atm == 0) return NULL;

    ... look for the atom "atm" in the property list and remove it ...
    if (!found) return NULL;

    // clean up the atom
    if (HIWORD(pszName)) GlobalDeleteAtom(atm);
}

void CleanPropertiesWhenWindowIsDestroyed(HWND hwnd)
{
    for (each property on the window) {
        if (atm >= MAXINTATOM) GlobalDeleteAtom(atm);
    }
    .. delete memory used for recording properties ...
}

First, let's look at properties set and removed via integer atoms. These are simple: When setting the property, we just add it to the property list, and when removing the property, we remove it. Nothing fancy going on here.

Similarly, there's nothing particularly exciting going on if a property is set and removed by name. When setting the property, we use GlobalAddAtom to convert the string to an atom (incrementing the reference count), and when removing it, we use GlobalDeleteAtom to clean it up (decrementing the reference count and removing the atom if the reference count goes to zero).

Finally, when a window is destroyed with outstanding properties, we clean them up by calling GlobalDeleteAtom on all the string atoms, counteracting the GlobalAddAtom we performed when we added the property.

So what's the big deal? Looks great, right?

See if you can find the hole in this implementation.

Hint 1: There are actually three ways of adding and removing properties from a window, not the two I led you to believe.

Hint 2: What happens if you mix and match these three methods?

Hint 3: What happens to each of the three types of properties when the window manager is forced to clean them up?

These problems with properties were fixed a long time ago, but old-timers remain wary of adding named properties by string atom. It's one of those superstitions.

Comments (8)
  1. MM says:

    "See if you can find the hole in this implementation."

    Looks like CleanPropertiesWhenWindowIsDestroyed() destroys all string atoms, not just the ones that were created internally by SetProp("string") calls.

  2. waleri says:

    Hmm, local atom?

    Docs says it should be global, but can SetProp() tell the difference?

  3. MadQ1 says:

    I’m assuming that an ATOM’s reference count really is global, meaning that one process could GlobalAddAtom, and another process could GlobalDeleteAtom it out of existence. The documentation isn’t specific about this (and I don’t think it needs to be, but I’m too lazy right now to write a program to find out.)

    That being the case, using window properties as implemented above, it’s possible that an application or the window manager could accidentally delete a registered clipboard format or window message.

    This could lead to two applications trying to communicate, but they could each have a different ATOM value for a registered window message or clipboard format. Ouch!

  4. Yoni says:

    Two guesses:

    1. Suppose you write

    hwnd1 = CreateWindow(..);

    hwnd2 = CreateWindow(..);

    SetProp(hwnd1, "evil_prop", 0);

    evil_prop_atom = GlobalFindAtom("evil_prop");

    SetProp(hwnd2, evil_prop_atom, 0);

    DestroyWindow(hwnd1);

    DestroyWindow(hwnd2);

    Unless I’m mistaken, this will double-free the property.

    1. If I’m reading your pseudocode correctly, you can use integer atoms as properties (i.e. pass "#32770" as the property name). This fits in with your first hint better than my previous guess, but I don’t see how it leads to a hole.
  5. Unknown says:

    You say "the fact that adding properties by atom is really a hole in the original implementation". So the intended implementation was just to allow normal strings and MAKEINTATOM(integer_atom) as pszName argument.

    Now, why is the first line like:

    ATOM atm = HIWORD(pszName) ? GlobalAddAtom(pszName) : LOWORD(pszName);

    and not just like:

    ATOM atm = GlobalAddAtom(pszName);

    ?

    This way pszName is still allowed to be MAKEINTATOM(integer_atom) and no hole would have existed.

  6. Unknown says:

    May it be the source of the "hole in the original implementation" was actually in GlobalAddAtom/AddAtom?

    Maybe the original implementation always returned pszName directly when pszName was specified as MAKEINTATOM() without checking if the value specified was actually < MAXINTATOM?

    Am I right?

  7. Ulric says:

    I’ve never had use for SetProp(), it’s something that can be done, or is already being done, other ways.   But some code of our application has used it.

    Is the code above suposed to be pseudo code of the implementation of SetProp/RemoveProp in Windows 3.x?  It has been my understanding from MSDN (and Bounds Checker – god bless its soul) that the properties added to a HWND leak if you don’t remove them before destruction.  So what’s this CleanPropertiesWhenWindowIsDestroyed ?

  8. Leo Davidson says:

    When you say the problems were long solved does that mean it’s safe to call SetProp with atoms on modern (NT-based) versions of Windows, or is there still a gotcha which means it’s better to stick to string property names even now?

    MSDN says you can pass an atom for the lpString argument so it’s no secret. I do this when subclassing windows (storing the original winproc in a property) as I assumed it would be faster to convert the string to an atom once myself instead of making the OS do it twice for every subclassed control.

    If there’s a risk that something will go wrong then I’ll switch to using strings as robustness is is more important than speeding up window opening by a fraction of a millisecond.

Comments are closed.

Skip to main content