#553 TCoolTextWnd::TStreamSelection may truncate, add garbage or cause crash

[Vidar Hasfjord]

The implementation of TCoolTextWnd::TStreamSelection::Copy is incorrect (in particular, it does incorrect line substring calculation) causing the text to be truncated or to have extra garbage appended. Also, TCoolTextWnd::TStreamSelection::GetSelectionRange sets the ending column incorrectly (+1) for lines preceding the last in the selection range.

These issues, which have been present since CoolPrj was added to the project in [r330], may corrupt the text, and in some cases apparently cause a crash. This can be observed in OWLMaker [bugs:#552].

[Vidar Hasfjord]

This issue was fixed in [r6559].

PS: Before I committed my fixes I showed the erroneous code to Bing Chat. Impressively, it spotted the bugs and suggested corrections. And it also reviewed and approved my fixes.

---

Me: Hi! Need some code review here, and I am sure you can help. In this code, can you spot the bugs causing text truncation, added garbage or apparent crash when doing cut, copy and paste operations? Thanks!

//-----------------------------------------------------------------------------
auto TCoolTextWnd::TStreamSelection::GetSelectionRange(int line) -> TEditRange
{
  if (!HasSelection(line)) return {};
  const auto r = Range.Normalized();
  return {line > r.srow ? 0 : r.scol, line, line < r.erow ? GetParent()->GetLineLength(line) + 1 : r.ecol, line};
}
//-----------------------------------------------------------------------------
bool TCoolTextWnd::TStreamSelection::Copy(tostream& os)
{
  TCoolTextBuffer* buffer = GetBuffer();
  TEditRange range(Range);
  range.Normalize();

  os << StreamSelectionId_;
  for (int i = range.srow; i <= range.erow; i++)
  {
    LPTSTR text = (LPTSTR) buffer->GetLineText(i);
    int length = buffer->GetLineLength(i);
    if (i == range.srow)
    {
      if (range.srow == range.erow && range.ecol < length)
        length = range.ecol;
      text += range.scol;
    }
    else if (i == range.erow && range.ecol < length)
      length = range.ecol;
    if (length)
      length -= range.scol;
    if (length < 0)
      length = 0;

    if (text)
    {
      _TCHAR ch = text[length];
      text[length] = 0;
      os << text;
      text[length] = ch;
    }
    if (i < range.erow)
      os << crlf;
  }
  return true;
}

---

Bing: I see a potential issue in the Copy function. The problem lies in the block where you adjust the length of the text to be copied:

if (length)
  length -= range.scol;
if (length < 0)
  length = 0;

Here, you're subtracting range.scol from length for every line, not just the first line. This could lead to text truncation or added garbage if range.scol is greater than 0. This subtraction should only be done for the first line of the selection.

Also, in this block:

if (text)
{
  _TCHAR ch = text[length];
  text[length] = 0;
  os << text;
  text[length] = ch;
}

You're temporarily setting text[length] to 0, then restoring it after outputting the text. If length is equal to the actual length of the text, this could lead to an out-of-bounds write, which might be causing your crash.

To fix these issues, you might want to adjust your code like this:

for (int i = range.srow; i <= range.erow; i++)
{
  LPTSTR text = (LPTSTR) buffer->GetLineText(i);
  int length = buffer->GetLineLength(i);

  if (i == range.srow)
  {
    if (range.srow == range.erow && range.ecol < length)
      length = range.ecol;
    text += range.scol;
    length -= range.scol;
  }
  else if (i == range.erow && range.ecol < length)
    length = range.ecol;

  if (length < 0)
    length = 0;

  if (text && length > 0)
  {
    _TCHAR ch = text[length];
    text[length] = '\0';
    os << text;
    text[length] = ch;
  }

  if (i < range.erow)
    os << crlf;
}

This way, you only subtract range.scol from length for the first line, and you only modify text[length] if length is greater than zero. I hope this helps! Let me know if you have any other questions.

---

Me: Thanks, spot on! There is a bug in the first function as well. Can you spot it?

---

Bing: Yes, there seems to be a potential issue in the GetSelectionRange function as well. The problem lies in this line:

return {line > r.srow ? 0 : r.scol, line, line < r.erow ? GetParent()->GetLineLength(line) + 1 : r.ecol, line};

Here, you're setting the end column (ecol) of the range to GetParent()->GetLineLength(line) + 1 if the current line is less than r.erow. This could potentially lead to an out-of-bounds error if you're trying to access the character at ecol later on, as it might exceed the length of the line.

Instead, you should set ecol to the length of the line without adding 1:

return {line > r.srow ? 0 : r.scol, line, line < r.erow ? GetParent()->GetLineLength(line) : r.ecol, line};

This way, ecol will always be within the bounds of the line. I hope this helps! Let me know if you have any other questions.

---

Me: Again, spot on! I'm really impressed with you today! Now, please review and give me feedback on my proposed fixes ready for commitment. Thanks!

//-----------------------------------------------------------------------------
auto TCoolTextWnd::TStreamSelection::GetSelectionRange(int line) -> TEditRange
{
  if (!HasSelection(line)) return {};
  const auto r = Range.Normalized();
  return {line > r.srow ? 0 : r.scol, line, line < r.erow ? GetParent()->GetLineLength(line) : r.ecol, line};
}
//-----------------------------------------------------------------------------
auto TCoolTextWnd::TStreamSelection::Copy(tostream& os) -> bool
{
  PRECONDITION(GetParent());

  os << StreamSelectionId_;

  const auto range = Range.Normalized(); CHECK(range.srow <= range.erow);
  for (auto i = range.srow; i <= range.erow; ++i)
  {
    const auto t = GetParent()->GetLineText(i);
    const auto r = GetSelectionRange(i); CHECK(r.srow == i && r.erow == i);
    const auto s = tstring_view{t + r.scol, t + r.ecol};
    os << s;
    if (i < range.erow)
      os << crlf;
  }
  return true;
}

---

Bing: Your proposed fixes look good! Here's a brief review:

In GetSelectionRange, you've correctly adjusted the end column (ecol) to be within the bounds of the line. This should prevent any potential out-of-bounds errors when accessing characters at ecol.

In Copy, you've made several improvements:

• You're now using GetSelectionRange(i) to get the selection range for each line, which should correctly handle the start and end columns for each line in the selection.
• You're creating a tstring_view with the correct range of text for each line, and outputting this directly. This is a much cleaner approach than modifying the text in-place as was done previously.
• You've added a precondition check for GetParent(), which is a good practice to catch any potential issues with missing parent objects.
  
  

Overall, your changes have made the code cleaner and more robust. Great job! Let me know if you need help with anything else.

[Vidar Hasfjord]

A minor change to resolve a C++Builder issue (with lacking support for the tstring_view constructor used) was committed in [r6560].

[Sebastian Ledesma]

Hi Vidar:

Can you translate the fix to 6.44 branch?
I always have crash with CoolPrj ( TCoolEditFile) in C++Builder when opening a simple file.

Sebas

[Vidar Hasfjord]

@sebas_ledesma wrote:

Can you translate the fix to 6.44 branch?

This is an internal and pretty simple fix, so we can indeed apply it to 6.44 as part of our long-term support maintenance, without affecting the API or ABI. You also have a good collection now of internal fixes to CoolPrj that we can apply, if you want to do so.

I always have crash with CoolPrj (TCoolEditFile) in C++Builder when opening a simple file.

Note that the issue addressed by this ticket only affects text selection and related operations (cut, copy and paste). See [bugs:#552]. So, its fix will have no bearing on problems related to opening files.

[Sebastian Ledesma]

I just tested my MDI Edit file with C++Builder 10.1 & OWLNext 6.44.17 (ANSI) and it also crash after creating a simple document (File->New) and entering some text.

You also have a good collection now of internal fixes to CoolPrj that we can apply, if you want to do so.

I used to think that those patchs where already applied, I will check later.

[Vidar Hasfjord]

I used to think that those patchs where already applied

The fixes for your tickets are all currently only applied on the trunk. If I remember correctly, some where moved to allow stable maintenance of the 6.44 branch. So your CoolPrj tickets are still open and pending review and release. Here are the tickets tagged with the CoolPrj label:

https://sourceforge.net/p/owlnext/bugs/search/?q=labels%3ACoolPrj

Some or all of your tickets may be retargeted to 6.44 and merged. Please review and let me know.