#593 TDialogRes::GetText crashes for extended dialog template

[Vidar Hasfjord]

Currently, TDialogRes::GetText fails for extended dialog templates (DLGTEMPLATEEX), since in this case the implementation is missing, and consequently, pointers are left uninitialised:

int TDialogRes::GetText(LPTSTR buffer, int size, TDlgResText which) const
{
  LPCWSTR p, pMenu, pClass, pCaption;

  if (!IsDialogEx()) {
    /*...Initialise pMenu, pClass and pCaption...*/
  }
  //else{  Y.B. Finish ??????????????????????????????????????????????????????
  //    DLGTEMPLATEEX* dlgTemplateEx = GetTemplateEx();
    // !BB Get info about DLGEX!
  //}

  switch (which) {
    case drtMenuName:   p = pMenu;    break;
    case drtClassName:  p = pClass;   break;
    case drtCaption:    p = pCaption; break;
    default:
      return 0;
  };

  /*...Use pointer p...*/
}

[Ognyan Chernokozhev]

A series of posts about dialog templates from Raymon Chen's blog:

The evolution of dialog templates – Introduction

The evolution of dialog templates – 16-bit Classic Templates

The evolution of dialog templates – 32-bit Classic Templates

The evolution of dialog templates – 16-bit Extended Templates

The evolution of dialog templates – 32-bit Extended Templates

The evolution of dialog templates – Summary

[Ognyan Chernokozhev]

After doing some research, the code in TDialogRes seems to be complete nonsense.

There is no such thing as RT_NEWDIALOG. I don't know where Borland got that idea.

Instead, both old and new dialog templates are loaded using RT_DIALOG , and the way to distinguish them is by checking the first four bytes of the template. See Raymond Chen's posts.

Also, the definition of DLGTEMPLATEEX was not using the correct alignment and ended being the wrong size.

[Ognyan Chernokozhev]

Code fixed in [r8649].

[Ognyan Chernokozhev]

[r8650] added an example to the Classes project to test TDialogRes with both old and new dialog templates.

[Vidar Hasfjord]

@jogybl wrote:

added an example to the Classes project to test TDialogRes

Files are missing ("dialogrestest.(h|cpp)").

[Ognyan Chernokozhev]

Thanks, pushed them.

[Vidar Hasfjord]

@jogybl wrote:

After doing some research, the code in TDialogRes seems to be complete nonsense.

Good work on the rewrite and the test!

[Ognyan Chernokozhev]

The GetText function is now replaced with a GetDialogData function that returns a struct with the parsed members of the dialog template.

[Vidar Hasfjord]

Hi Ognyan,

Good work on fixing this! This kind of low-level binary parsing, with potential alignment issues and pointer arithmetic corner cases, is tricky to get right, so good of you to add a test case in Classes — which seems to work nicely!

Have you had your code reviewed by AI?

I'm trying not to get involved on the trunk, so I just asked Windows Copilot (Smart) to have a look at your InitDialogData, and it complained about a few issues with alignment, type casts and pointer arithmetic in places. I didn't get into the weeds, but noted that it made a false statement at least in one place, so the review clearly wasn't fully reliable. But Copilot suggested generating some synthetic test cases that exercise the tricky corner cases, which is a good idea. The currently tested dialogs may be deceptively regular.

[Ognyan Chernokozhev]

I tried to use AI to generate the code for this, and some of the work was done by it, but I had to make a number of corrections, including I think omitting the star in statements like (ushort)*(pControlClass + 1).

The currently tested dialogs may be deceptively regular.

Yeah, I am not sure the code would be handling correctly the case in which controls have extra bytes. Will need more research when I get back to this.

[Vidar Hasfjord]

@jogybl wrote:

I tried to use AI to generate the code for this, and some of the work was done by it

Even if you use AI to generate code, having AI review the code in a fresh prompt can be useful. Maybe use a different AI for the review, and use the smartest AI you have access to (in thinking mode, so that it thinks before it speaks).

Here's the code in "InitDialogData" I couldn't unsee:

  if ((pItems - p) % 2 == 0)
  {
    // Align to DWORD boundary
    ++pItems;
  }

This baffled me and Copilot alike, and it took some time before I could convince myself (and Copilot) that it works.

This code does not do absolute memory address alignment, but instead aligns the offset from p, which in turn is presumed to be at a WORD aligned but DWORD unaligned offset from the base address of the dialog template. The presumption that the offset from base to p is DWORD unaligned relies on sizeof(DLGTEMPLATE) and sizeof(DLGTEMPLATEEX) both being an odd multiple of sizeof(WORD), just by coincidence.

If keeping the code as is, consider asserting these presumptions in the code:

PRECONDITION(sizeof(DLGTEMPLATE) % sizeof(DWORD) == sizeof(WORD));
PRECONDITION(sizeof(DLGTEMPLATEEX) % sizeof(DWORD) == sizeof(WORD));
CHECK((p - base) % 2 == 1); // Offset at p is DWORD unaligned.
if ((pItems - p) % 2 == 0) // Is offset at pItems DWORD unaligned?
{
  // Align to DWORD boundary.
  ++pItems;
}

Or simplify by checking the offset from the base instead:

if ((pItems - base) % 2 == 1) // Is offset at pItems DWORD unaligned?
{
  // Align to DWORD boundary.
  ++pItems;
}

Should you align the offset or the absolute memory address? If the base address is always DWORD aligned it doesn't matter, but absolute memory address alignment is more conventional and perhaps easier to understand. However, if you want to allow the template to be located at an unaligned address, then offset alignment will ensure the code still works.

Note that the Windows API (CreateDialogIndirectParam) requires that the template is DWORD aligned:

In a standard dialog box template, the DLGTEMPLATE structure and each of the DLGITEMTEMPLATE structures must be aligned on DWORD boundaries. [...] In an extended dialog box template, the DLGTEMPLATEEX header and each of the DLGITEMTEMPLATEEX control definitions must be aligned on DWORD boundaries.

Hence my recommendation would be to just assert this requirement with a PRECONDITION and use conventional absolute memory address alignment.

Here are some other good suggestions by Copilot:

• Use a byte pointer for binary parsing — do all pointer arithmetic in bytes, and cast each field to the appropriate data type as you go (using modern C++ casts, not C casts).
• Do bounds checking — make sure you don't overrun the buffer (see TResource::GetSize).
• Use std::variant<tstring, uint> — for fields that can either be string or ordinal.
  
  

I am not sure the code would be handling correctly the case in which controls have extra bytes.

Good call. Due to the LPCWSTR type of the pointer here, the number of bytes skipped is always even (rounded down), and the last extra byte is not skipped if the count is odd (unless it is coincidentally skipped by the subsequent alignment).

pItems += controlData.ExtraBytesCount / sizeof(ushort);

Here is a code rewrite that circumvents this problem, while simplifying things, adding assertions and preventing buffer overflow:

void TDialogRes::InitDialogData()
{
  PRECONDITION(Resource && Resource->GetSize() >= sizeof(DLGTEMPLATE));

  // Define some helpers.
  //
  using TParsingSpan = span<const std::byte>;
  const auto parseHeader = [&](TParsingSpan& s) { ... }
  const auto parseItem = [&](TParsingSpan& s) { ... }

  // Do the parsing.
  //
  const auto t = Resource->operator DLGTEMPLATE*();
  auto s = TParsingSpan{reinterpret_cast<const std::byte*>(t), GetSize()};
  DialogData = parseHeader(s);
  CHECK(IsAligned_<WORD>(s));
  DialogData.MenuName = ConvertToString_(ParseStringOrOrdinal_(s));
  DialogData.ClassName = ConvertToString_(ParseStringOrOrdinal_(s));
  DialogData.Caption = ParseString_(s);
  if (DialogData.Style & DS_SETFONT)
  {
    DialogData.FontSize = ParseField_<WORD>(s);
    if (DialogData.IsDialogEx)
    {
      [[maybe_unused]] const auto weight = ParseField_<WORD>(s);
      [[maybe_unused]] const auto italic = ParseField_<BYTE>(s);
      [[maybe_unused]] const auto charset = ParseField_<BYTE>(s);
    }
    DialogData.FontName = ParseString_(s);
  }
  for (auto i = 0; i < DialogData.ItemCount; ++i)
  {
    Align_<DWORD>(s);
    DialogData.Controls.push_back(parseItem(s));
  }
}

Here is the implementation of the local helpers:

  const auto parseHeader = [&](TParsingSpan& s) -> TDialogData
  {
    PRECONDITION(IsAligned_<DWORD>(s));
    auto dialogData = TDialogData{};

    const auto initHeaderData = [&](const auto& t, bool isDialogEx, ushort itemCount, uint32 exStyle = 0, uint32 helpId = 0)
    {
      dialogData.IsDialogEx = isDialogEx;
      dialogData.Style = t.style;
      dialogData.Rect = {{t.x, t.y}, TSize{t.cx, t.cy}};
      dialogData.ItemCount = itemCount;
      dialogData.ExStyle = exStyle;
      dialogData.HelpId = helpId;
    };

    if (IsDialogEx()) 
    {
      const auto* t = reinterpret_cast<const DLGTEMPLATEEX*>(s.data());
      if (s.size() < sizeof(*t)) throw TXOwl{to_tstring(__func__) + _T(": Truncation")};
      initHeaderData(*t, true, t->itemCount, t->exStyle, t->helpId);
      s = s.subspan(sizeof(*t));
    }
    else 
    {
      const auto* t = reinterpret_cast<const DLGTEMPLATE*>(s.data());
      if (s.size() < sizeof(*t)) throw TXOwl{to_tstring(__func__) + _T(": Truncation")};
      initHeaderData(*t, false, t->cdit, t->dwExtendedStyle);
      s = s.subspan(sizeof(*t));
    }

    return dialogData;
  };

  const auto parseItem = [&](TParsingSpan& s)
  {
    PRECONDITION(IsAligned_<DWORD>(s));
    auto controlData = TDialogControlData{};

    const auto initHeaderData = [&](const auto& t, uint32 exStyle = 0, uint32 helpId = 0)
    {
      controlData.Id = t.id;
      controlData.Style = t.style;
      controlData.ExStyle = exStyle;
      controlData.Rect = {{t.x, t.y}, TSize{t.cx, t.cy}};
      controlData.HelpId = helpId;
    };

    // Parse header.
    //
    if (DialogData.IsDialogEx)
    {
      const auto t = reinterpret_cast<const DLGITEMTEMPLATEEX*>(s.data());
      if (s.size() < sizeof(*t)) throw TXOwl{to_tstring(__func__) + _T(": Truncation")};
      initHeaderData(*t, t->exStyle, t->helpID);
      s = s.subspan(sizeof(*t));
    }
    else
    {
      const auto t = reinterpret_cast<const DLGITEMTEMPLATE*>(s.data());
      if (s.size() < sizeof(*t)) throw TXOwl{to_tstring(__func__) + _T(": Truncation")};
      initHeaderData(*t, t->dwExtendedStyle);
      s = s.subspan(sizeof(*t));
    }

    // Parse strings and extra bytes.
    //
    CHECK(IsAligned_<WORD>(s));
    controlData.ClassName = ConvertToControlClass_(ParseStringOrOrdinal_(s));
    controlData.Text = ConvertToString_(ParseStringOrOrdinal_(s));
    controlData.ExtraBytesCount = ParseField_<WORD>(s);
    s = s.subspan(controlData.ExtraBytesCount);
    return controlData;
  };

And here are the global helpers:

namespace {

template <class T>
auto IsAligned_(span<const std::byte> s)
{
  const auto a = reinterpret_cast<uintptr_t>(s.data());
  return a % sizeof(T) == 0;
};

template <class T>
void Align_(span<const std::byte>& s)
{
  const auto a = reinterpret_cast<uintptr_t>(s.data());
  const auto n = sizeof(T);
  const auto misalignment = a % n;
  if (misalignment == 0) return; // Already aligned.
  const auto offset = n - misalignment;
  s = s.subspan(offset);
}

template <class T>
auto ParseField_(span<const std::byte>& s)
{
  if (s.size() < sizeof(T)) throw TXOwl{to_tstring(__func__) + _T(": Truncation")};
  const auto r = *reinterpret_cast<const T*>(s.data());
  s = s.subspan(sizeof(T));
  return r;
}

auto ParseString_(span<const std::byte>& s)
{
  PRECONDITION(IsAligned_<wchar_t>(s));
  const auto p = reinterpret_cast<const wchar_t*>(s.data());
  const auto maxCount = s.size() / sizeof(wchar_t);
  const auto n = wcsnlen_s(p, maxCount);
  if (n >= maxCount) throw TXOwl{to_tstring(__func__) + _T(": Truncation")};
  s = s.subspan((n + 1) * sizeof(wchar_t));
  return to_tstring(wstring_view{p, n});
}

auto ParseStringOrOrdinal_(span<const std::byte>& s) -> variant<tstring, WORD>
{ 
  PRECONDITION(IsAligned_<WORD>(s));
  auto peek = s;
  if (ParseField_<WORD>(peek) == 0xFFFF) // Ordinal marker?
  {
    s = peek;
    return ParseField_<WORD>(s);
  }
  return ParseString_(s);
}

auto ConvertToString_(const variant<tstring, WORD>& v)
{
  return holds_alternative<tstring>(v) ? get<tstring>(v) : _T('#') + to_tstring(get<WORD>(v));
}

auto ConvertToControlClass_(const variant<tstring, WORD>& v)
{
  const auto lookupControlClass = [](WORD ordinal) -> tstring
  {
    switch (ordinal)
    {
    case 0x0080: return _T("Button");
    case 0x0081: return _T("Edit");
    case 0x0082: return _T("Static");
    case 0x0083: return _T("ListBox");
    case 0x0084: return _T("ScrollBar");
    case 0x0085: return _T("ComboBox");
    default: return ConvertToString_(ordinal);
    }
  };

  return holds_alternative<tstring>(v) ? get<tstring>(v) : lookupControlClass(get<WORD>(v));
}

} // namespace

[Ognyan Chernokozhev]

Thanks. I have applied and tested the changes in [r8746].

[Vidar Hasfjord]

@jogybl wrote:

Thanks. I have applied and tested the change in [r8746].

Good to hear it passed testing! I made a minor refactoring of ConvertToString_, separating the control class lookup into a new function ConvertToControlClass_, just for better separation of concerns, but this is not that important here.

Regarding [r8747], there is an issue in your change of the test for the font fields:

-  if (DialogData.Style & DS_SETFONT)
+  if ((DialogData.Style & DS_SETFONT) || (DialogData.Style & DS_SHELLFONT))

Since DS_SHELLFONT contains multiple flags (DS_SETFONT | DS_FIXEDSYS), this does not do what you intend; it will evalute to true even if just DS_FIXEDSYS is set. You can change it to (DialogData.Style & DS_SETFONT) || (DialogData.Style & DS_SHELLFONT) == DS_SHELLFONT to fix this, but it just ends up being redundant. Your original code testing only the DS_SETFONT flag is sufficient.

[Vidar Hasfjord]

Another possible issue in [r8747] is this test:

DialogData.FontItalic = ParseField_<BYTE>(s) == TRUE;

This means that e.g. with the parsed BYTE equal to 0xFF (all bits set), the expression evaluates to false, since the BYTE is not equal to TRUE (0x01). This is usually fixed by always writing != FALSE instead. But this one is very easy to forget, as I can attest to myself!