#495 TWindow::EvCtlColor unsafely relies on TBrush cache

[Vidar Hasfjord]

Since OWL 5, at least, TWindow::EvCtlColor has returned a handle to a TBrush created locally within the function scope. This is usually an awful programming mistake, but here it is intended to work.

::SetBkColor(hDC, bkgndColor);
return TBrush{bkgndColor}; // HBRUSH will stay in cache.

As the code comment points out, this awful code relies on the fact that TBrush is implemented as having shared ownership of the underlying handle, which is also entered into a 16-entry cache of solid brushes by the TBrush constructor, if applicable and space allows. However, relying on the cache is unsafe, since it may run out of free space.

Having briefly reviewed the cache implementation in "brush.cpp (sourceforge.net)", it looks like it is meant to have support for a replacement policy. A replacement policy, assuring room for the latest entry, would ensure that the handle returned by EvCtlColor at least remained alive throughout the processing of the message. However, as I understand it, there is no replacement implemented, contrary to what the code comment claims. The cache will simply refuse another entry, if full. If that happens for the brush created in EvCtlColor, it will be destroyed at the end of the scope, and the handle will be dangling.

//
// Add a new entry to the front, pushing the last old one off the end.
//
bool
TBrushCache::AddRef(HANDLE handle, COLORREF color)
{
  // ...
  bool retval = true;
  if (Entries[Size-1].Handle){ // ...last slot occupied?...
    int i = Size-1;
    for(; i >= 0; i--){ // ...search for free slot...
      // if RefCnt == 0 no one use this
      if(!Entries[i].RefCnt){
        /// ...use this slot...
        break;
      }
    }
    if(i < 0) // free node not found
      retval = false;
  }
  else{
    /// ...last slot free, so use it...
  }
  // ...
}

I strongly suspect there is no one left active on the OWLNext project that properly understands the TBrush caching, as well as the overall GDI object sharing and reference counting scheme, on a detailed level, deep enough to properly trust and maintain it. So in Owlet, I am in the process of replacing it all with much simpler and maintainable code.

This particular issue was fixed in Owlet in [r5475] by adding a private member TWindow::BkgndBrush to hold the returned brush.

[Ognyan Chernokozhev]

Claude Code:

---

Merged into trunk in [r8974] adapted from the Owlet fix [r5475]. Trunk uses std::unique_ptr<TBrush> BkgndBrush, matching the existing TWindow::Font member, rather than std::optional. The layout of TWindow has changed, so dependent code must be rebuilt.

A correction to the description: TBrushCache::AddRef does have a replacement policy. The code quoted above leaves out the body of the search loop. When the cache is full, AddRef evicts the last entry with RefCnt == 0 and deletes its handle. The cache refuses a new brush only if all 16 entries are held by live TBrush objects.

The bug is still real, and it shows up in two ways:

1. Cache full. All 16 entries are in use, for example because the application keeps 16 or more solid brushes of distinct non-system colours alive. The temporary TBrush in EvCtlColor then owns its handle and deletes it on return, so the control is given a deleted brush.
2. Eviction after return. The brush is cached, but its entry has RefCnt == 0 once the temporary is destroyed. The next uncached brush can evict it and delete the handle, and GDI may then reuse the handle value for a different object.

I checked this with a headless test that sends WM_CTLCOLORSTATIC to a TWindow with a background colour set, and checks the returned handle with GetObjectType/GetObject.

• Before the fix, case 1 returned a deleted handle (GetObjectType returned 0). In case 2 the handle had been reused by a brush of a different colour.
• After the fix, both cases return a live solid brush of the right colour.
• 200 messages with changing colours did not leak any GDI objects, and the TColor::None and TColor::Transparent paths are unchanged.