#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.