Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AtlasEngine: Fix a heap overflow bug #14275

Merged
merged 3 commits into from
Nov 2, 2022
Merged

AtlasEngine: Fix a heap overflow bug #14275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d794069
AtlasEngine: Fix a heap overflow bug
lhecker Oct 21, 2022
35c8029
Revert an unnecessary change
lhecker Oct 21, 2022
0982f68
Merge remote-tracking branch 'origin/main' into dev/lhecker/atlas-eng…
lhecker Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 24 additions & 19 deletions src/renderer/atlas/AtlasEngine.api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,30 +96,35 @@ constexpr HRESULT vec2_narrow(U x, U y, AtlasEngine::vec2<T>& out) noexcept

[[nodiscard]] HRESULT AtlasEngine::InvalidateScroll(const til::point* const pcoordDelta) noexcept
{
const auto delta = pcoordDelta->Y;
if (delta == 0)
{
return S_OK;
}

_api.scrollOffset = gsl::narrow_cast<i16>(clamp<int>(_api.scrollOffset + delta, i16min, i16max));

// InvalidateScroll() is a "synchronous" API. Any Invalidate()s after
// a InvalidateScroll() refer to the new viewport after the scroll.
// --> We need to shift the current invalidation rectangles as well.

_api.invalidatedCursorArea.top = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedCursorArea.top + delta, u16min, u16max));
_api.invalidatedCursorArea.bottom = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedCursorArea.bottom + delta, u16min, u16max));

if (delta < 0)
if (const auto delta = pcoordDelta->X)
{
_api.invalidatedRows.x = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedRows.x + delta, u16min, u16max));
_api.invalidatedRows.y = _api.cellCount.y;
_api.invalidatedCursorArea.left = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedCursorArea.left + delta, u16min, u16max));
_api.invalidatedCursorArea.right = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedCursorArea.right + delta, u16min, u16max));

_api.invalidatedRows = invalidatedRowsAll;
}
else

if (const auto delta = pcoordDelta->Y)
{
_api.invalidatedRows.x = 0;
_api.invalidatedRows.y = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedRows.y + delta, u16min, u16max));
_api.scrollOffset = gsl::narrow_cast<i16>(clamp<int>(_api.scrollOffset + delta, i16min, i16max));

_api.invalidatedCursorArea.top = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedCursorArea.top + delta, u16min, u16max));
_api.invalidatedCursorArea.bottom = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedCursorArea.bottom + delta, u16min, u16max));

if (delta < 0)
{
_api.invalidatedRows.x = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedRows.x + delta, u16min, u16max));
_api.invalidatedRows.y = _api.cellCount.y;
}
else
{
_api.invalidatedRows.x = 0;
_api.invalidatedRows.y = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedRows.y + delta, u16min, u16max));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is way less egregious if you suppress whitespace changes.

This block of changes is unrelated to this PR but represent an earlier attempt at fixing it. I left it in because it'll help with implementing horizontal scrolling in the future.


return S_OK;
Expand Down Expand Up @@ -176,8 +181,8 @@ constexpr HRESULT vec2_narrow(U x, U y, AtlasEngine::vec2<T>& out) noexcept
[[nodiscard]] HRESULT AtlasEngine::UpdateViewport(const til::inclusive_rect& srNewViewport) noexcept
{
const u16x2 cellCount{
gsl::narrow_cast<u16>(srNewViewport.Right - srNewViewport.Left + 1),
gsl::narrow_cast<u16>(srNewViewport.Bottom - srNewViewport.Top + 1),
gsl::narrow_cast<u16>(std::max(1, srNewViewport.Right - srNewViewport.Left + 1)),
gsl::narrow_cast<u16>(std::max(1, srNewViewport.Bottom - srNewViewport.Top + 1)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevents an orderly/expected crash when you resize the conhost window down to 0px height.

};
if (_api.cellCount != cellCount)
{
Expand Down
43 changes: 35 additions & 8 deletions src/renderer/atlas/AtlasEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,28 +342,51 @@ CATCH_RETURN()
return S_OK;
}

[[nodiscard]] HRESULT AtlasEngine::PaintBufferLine(const gsl::span<const Cluster> clusters, const til::point coord, const bool fTrimLeft, const bool lineWrapped) noexcept
[[nodiscard]] HRESULT AtlasEngine::PaintBufferLine(gsl::span<const Cluster> clusters, til::point coord, const bool fTrimLeft, const bool lineWrapped) noexcept
try
{
const auto x = gsl::narrow_cast<u16>(clamp<int>(coord.X, 0, _api.cellCount.x));
const auto y = gsl::narrow_cast<u16>(clamp<int>(coord.Y, 0, _api.cellCount.y));

if (_api.lastPaintBufferLineCoord.y != y)
{
_flushBufferLine();
}

_api.lastPaintBufferLineCoord = { x, y };
_api.bufferLineWasHyperlinked = false;
// _api.bufferLineColumn contains 1 more item than _api.bufferLine, as it represents the
// past-the-end index. It'll get appended again later once we built our new _api.bufferLine.
if (!_api.bufferLineColumn.empty())
{
_api.bufferLineColumn.pop_back();
}

// Due to the current IRenderEngine interface (that wasn't refactored yet) we need to assemble
// the current buffer line first as the remaining function operates on whole lines of text.
// `TextBuffer` is buggy and allows a `Trailing` `DbcsAttribute` to be written
// into the first column. Since other code then blindly assumes that there's a
// preceding `Leading` character, we'll get called with a X coordinate of -1.
//
// This block can be removed after GH#13626 is merged.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will never remember this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, we don't really have to. This is a small, self-contained snippet and so I feel like it can be removed whenever. But I'll add a note to make this change in my ROW PR.

if (coord.X < 0)
{
if (!_api.bufferLineColumn.empty())
size_t offset = 0;
for (const auto& cluster : clusters)
{
_api.bufferLineColumn.pop_back();
offset++;
coord.X += cluster.GetColumns();
if (coord.X >= 0)
{
_api.bufferLine.insert(_api.bufferLine.end(), coord.X, L' ');
_api.bufferLineColumn.insert(_api.bufferLineColumn.end(), coord.X, 0u);
break;
}
}

clusters = clusters.subspan(offset);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix.


const auto x = gsl::narrow_cast<u16>(clamp<int>(coord.X, 0, _api.cellCount.x));

// Due to the current IRenderEngine interface (that wasn't refactored yet) we need to assemble
// the current buffer line first as the remaining function operates on whole lines of text.
{
auto column = x;
for (const auto& cluster : clusters)
{
Expand All @@ -379,9 +402,13 @@ try
_api.bufferLineColumn.emplace_back(column);

const BufferLineMetadata metadata{ _api.currentColor, _api.flags };
FAIL_FAST_IF(column > _api.bufferLineMetadata.size());
std::fill_n(_api.bufferLineMetadata.data() + x, column - x, metadata);
}

_api.lastPaintBufferLineCoord = { x, y };
_api.bufferLineWasHyperlinked = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block moved down since I'm computing x late now.


return S_OK;
}
CATCH_RETURN()
Expand Down