#494 RestoreDC causes TDC state corruption and crash

[Vidar Hasfjord]

TDC::RestoreDC has a horrible bug, which has lingered since OWL 5, at least. It does not play nice with the "orphan control" performed by TDC. In fact, RestoreDC is just a thin wrapper of the Windows API function RestoreDC. It is completely oblivious to the state tracking done by TDC as part of the orphan control.

This means that any references that may have been acquired by TDC, to the objects deselected by RestoreDC, will not be released, and references to the objects reselected by RestoreDC will not be acquired. There may hence be undetected orphans, while detected orphans will dangle and pile up. The following simple loop crashes in my test at about 10000 iterations:

for (...)
{
  const auto stateId = dc.SaveDC();
  const auto pen = TPen{...};
  dc.SelectObject(pen);
  dc.MoveTo(...);
  dc.LineTo(...);
  dc.RestoreDC(stateId);
}

I find this issue quite puzzling. The pair of functions SaveDC and RestoreDC are very useful in nested graphics routines. Similar functions for saving and restoring system state are often found in other graphics systems (well, that is what I remember from what I was taught in college back around 1990). You would think they were often used. But I cannot find any mention in the discussion forum, nor in the mailing list, about their usage or any problems. No bug tickets. Nothing at all. Strange.

An Internet search found this post from 2001 in a discussion about "Object Destruction in MFC" by Doug Harris, who seems to have caught on to the problem: "OWL reference-counted its C++ GDI objects, which I thought was a fine idea. The only problem I recall (it's been 5 years) was that it didn't do anything special for SaveDC/RestoreDC, and I believe it needed to". Why, Doug, did you not file a bug report with OWLNext? I also found this post from 1998 at ExpertsExchange by Quinlan, who reports a crash issue with a code segment using SaveDC and RestoreDC (the reply/solution is not public, though). I also got many hits about SaveDC and RestoreDC in OWL teaching material, so you would think the functions would be in use.

Now that I have rediscovered this issue, I remember running into it as a young programmer over 25 years ago. This was long before OWLNext and my participation online, so I didn't seek any help. Not having figured out what the issue was, I stopped using SaveDC and RestoreDC altogether, and I haven't used these functions since — until now, as I seek to better understand and improve upon TDC. See [discussion:00f93e1a3a].

[Vidar Hasfjord]

Note that calling TDC::RestoreObjects before TDC::RestoreDC will prevent orphans piling up and the program crashing. However, the orphan control is still messed up, since references to the reselected objects are not acquired, meaning a handle may be deleted despite still being selected.

[Vidar Hasfjord]

This issue was fixed in Owlet in [r5445] as follows:

void TDC::RestoreDC(int savedStateStackIndex)
{
  const auto deselectedPen = GetCurrentObject<HPEN>();
  const auto deselectedBrush = GetCurrentObject<HBRUSH>();
  const auto deselectedFont = GetCurrentObject<HFONT>();
  const auto deselectedPalette = GetCurrentObject<HPALETTE>();
  const auto deselectedBitmap = GetCurrentObject<HBITMAP>();

  const auto ok = ::RestoreDC(GetHDC(), savedStateStackIndex);
  if (!ok) throw TXGdi{GetHDC(), "RestoreDC failed"};

  const auto restoreRef = [&](auto deselectedHandle, auto orgHandle)
  {
    OWL_CHECK(deselectedHandle);
    const auto restoredHandle = GetCurrentObject<decltype(deselectedHandle)>();
    OWL_CHECK(restoredHandle);
    if (restoredHandle == deselectedHandle) return; // Nothing to do.

    // We do not want to take up a reference to an original handle, i.e. a handle that was selected
    // before the TDC was constructed, because TDC does not do that at construction.
    //
    // A null orgHandle implies that the restored handle is the original value. Either it has never
    // been changed by a SelectObject call, or it has been restored by a call to the corresponding
    // restore function (RestorePen, etc.) or RestoreObjects (which restores all), and there has
    // been no calls to SelectObject since.
    // 
    if (orgHandle && restoredHandle != orgHandle)
      TGdiObject::RefInc(restoredHandle); // Take up a reference to the restored handle.
    TGdiObject::RefDec(deselectedHandle, false); // Give up our reference to the deselected handle.
  };
  restoreRef(deselectedPen, OrgPen);
  restoreRef(deselectedBrush, OrgBrush);
  restoreRef(deselectedFont, OrgFont);
  restoreRef(deselectedPalette, OrgPalette);
  restoreRef(deselectedBitmap, OrgBitmap);
}

Please review. If you can fault my logic, let me know. I would like this fix to be robust, so any feedback would be appreciated.

[Ognyan Chernokozhev]

Claude Code:

---

Hi @vattila, I have reviewed the fix in [r5445] and merged it into trunk in [r8975], with some corrections. As you asked for faults in the logic, here they are.

The invariant maintained by SelectObject and RestorePen etc. is: TDC holds a reference to the selected object if, and only if, OrgX is set and the selected object is not OrgX. Checked against that, r5445 has three holes:

1. The deselected handle is always released. If OrgX is null, or the deselected handle is OrgX, TDC never acquired a reference to it. If that original is an OWL object, e.g. a TFont selected by an outer TDC on the same HDC, its owner loses a reference, and the object may be deleted prematurely.

2. "A null orgHandle implies that the restored handle is the original value" does not hold. Counterexample: SelectObject(a); SaveDC(); RestorePen(); RestoreDC(id);. Now OrgPen is null, but the restored pen is a, not the original. No reference to a is acquired, so a can be deleted while selected, and since OrgPen stays null, the original pen is never restored. In this case, the deselected handle is the original, so it can simply be saved in OrgPen.

3. OrgX is not reset when RestoreDC brings back the original. After SaveDC(); SelectObject(a); RestoreDC(id);, the selected object is OrgX, but OrgX is still set. A later RestoreX, or the destructor, then releases a reference to the original that TDC never acquired. That is harmless for stock objects, but not for OWL objects.

Also, GetCurrentObject returns null on a metafile DC (TMetaFileDC), so the OWL_CHECKs fire there.

The trunk version, per object type, is:

const auto sync = [this](HANDLE deselected, uint objectType, auto& org)
{
  const auto restored = GetCurrentObject(objectType);
  if (!deselected || !restored || restored == deselected) return; // E.g. a metafile DC.

  const auto wasDeselectedHeld = org && deselected != org;
  if (!org) // The deselected object is the original object.
    org = static_cast<std::remove_reference_t<decltype(org)>>(deselected);
  if (restored != org)
    TGdiObject::RefInc(restored);
  if (wasDeselectedHeld)
    TGdiObject::RefDec(deselected, false);
  if (restored == org)
    org = nullptr; // The original object is back in place; nothing to restore.
};

I tested it with a headless harness (memory DC, v145 x64 debug Unicode):

• Your loop with temporary pens: before the fix, it leaked 9996 GDI handles and then threw. After the fix, it leaks none.
• All five object types in nested SaveDC levels: before the fix, the program aborted. After the fix, there are no leaks, and the originals are restored.
• Reference counts after nested and relative (-1) restores: before the fix, 5 counts were wrong. After the fix, all are correct.
• Restoring to an original font owned by an outer TDC: before the fix, the outer font lost a reference. After the fix, it is untouched.
• RestoreFont between SaveDC and RestoreDC: before the fix, the counts were wrong and the original font was not restored. After the fix, the test passes.
• A failed restore (invalid index) and TMetaFileDC passed both before and after the fix.

Cases 4 and 5 are the ones r5445 gets wrong, by my reading of the code. I did not run it.

There is a remaining limitation, which this fix does not address: the saved DC state holds no references. In SelectObject(a); SaveDC(); SelectObject(b); RestoreDC(id);, if a's owner has already been destroyed, the second SelectObject releases the last reference, and a is deleted while it is still referenced by the saved state. In my test, DeleteObject returns TRUE in that case, but the handle stays valid and is reselected by RestoreDC. That is undocumented GDI behaviour, and the object is presumably leaked. A robust solution would let SaveDC push references for the selected objects onto a stack, and let RestoreDC pop them. That requires an ABI change, so for now the documentation says that objects selected when SaveDC is called must outlive the corresponding RestoreDC.

Two related issues:

• TPrintPreviewDC does not override RestoreDC, so a restore does not reach the printer DC or resync the preview font.
• SelectStockObject does not save OrgX when it is null, and releases the original instead.

[Ognyan Chernokozhev]

Follow-up in [r8976]: the remaining limitation and the two related issues I mentioned are now fixed in trunk.

Saved states hold references. TDC::SaveDC acquires references to the reference-counted objects selected at that time and pushes them on a stack. RestoreDC releases them for the popped states, and the destructor releases any that remain (unbalanced SaveDC). So in SelectObject(a); SaveDC(); SelectObject(b); RestoreDC(id);, a is no longer deleted while the saved state refers to it, even if its owner is destroyed in the meantime. For relative indexes, RestoreDC probes the new depth of the context stack with a ::SaveDC/::RestoreDC(-1) pair, so states saved outside the TDC are accounted for. Only objects that are registered at SaveDC time are held, since an unregistered handle may be adopted and registered later.
TPrintPreviewDC overrides SaveDC/RestoreDC. It saves and restores the printer DC along with the screen DC, using a list of the paired levels, and then calls SyncFont and ReScale.
TDC::SelectStockObject maintains the original objects. If no original is saved, the deselected object is saved as the original (and not released); otherwise only a held reference is released. DEFAULT_PALETTE is now selected with ::SelectPalette, since ::SelectObject does not select palettes.
This adds members to TDC and TPrintPreviewDC, so it is an ABI break.

Tests (headless; v145 x64 debug Unicode), compared with [r8975]:

An object selected only in a saved state, after its owner is destroyed: with r8975, it was deleted (not registered). Now it is alive, and it is released after the restore.
Relative restores across an external ::SaveDC, and an unbalanced SaveDC at destruction: with r8975, there were no saved-state references. Now the counts are correct, and the references are released at destruction.
SelectStockObject with an original font owned by another TDC: with r8975, that font lost a reference, it was not restored, and DEFAULT_PALETTE was not selected. Now the test passes.
TPrintPreviewDC with Microsoft Print to PDF, 200 save/select/restore cycles: with r8975, the printer font and text color were not restored. Now both are restored, the preview font is in sync, and no GDI objects leak.
The earlier [r8975] tests still pass.