microsoft/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2025-12-10 00:48:23 -06:00
Code Issues Packages Projects Releases 138 Wiki Activity

Support valid out-of-bounds access in utextAccess (#17361)

Browse Source 
`utextAccess` apparently doesn't actually need to clamp the
`chunkOffset` to be in range of the current chunk. Also, I missed to
implement the part of the spec that says to leave the iterator on the
first/last chunk of the `UText` in case of an out-of-bounds index.

This PR fixes the issue by simply not returning early, doing a more
liberal clamp of the offset, and then checking whether it was in range.

As an aside, this also fixes a one-off bug when hovering URLs that
end on the very last cell of the viewport (or are cut off).

Closes #17343

## Validation Steps Performed
* Write an URL that wraps across the last 2 lines in the buffer
* Scroll 1 line up
* No assert ✅
* Hovering the URL shows the full, still visible parts of the URL ✅
This commit is contained in:
Leonard Hecker 2024-06-05 19:07:59 +02:00 committed by GitHub
parent ce0f8d6db2
commit 261a3fec7a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 43 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
src/buffer/out/UTextAdapter.cpp
View File

@ -186,12 +186,9 @@ catch (...)
static UBool U_CALLCONV utextAccess(UText* ut, int64_t nativeIndex, UBool forward) noexcept
try
{
if (nativeIndex < 0)
{
nativeIndex = 0;
}
auto neededIndex = nativeIndex;
// This will make it simpler for us to search the row that contains the nativeIndex,
// because we'll now only need to check for `start<=index<limit` and nothing else.
if (!forward)
{
neededIndex--;
@ -199,10 +196,12 @@ try
const auto& textBuffer = *static_cast<const TextBuffer*>(ut->context);
const auto range = accessRowRange(ut);
auto start = ut->chunkNativeStart;
auto limit = ut->chunkNativeLimit;
const auto startOld = ut->chunkNativeStart;
const auto limitOld = ut->chunkNativeLimit;
auto start = startOld;
auto limit = limitOld;
if (neededIndex < start || neededIndex >= limit)
if (neededIndex < startOld || neededIndex >= limitOld)
{
auto y = accessCurrentRow(ut);
std::wstring_view text;
@ -215,8 +214,7 @@ try
--y;
if (y < range.begin)
{
assert(false);
return false;
break;
}
const auto& row = textBuffer.GetRowByOffset(y);
@ -235,8 +233,7 @@ try
++y;
if (y >= range.end)
{
assert(false);
return false;
break;
}
const auto& row = textBuffer.GetRowByOffset(y);
@ -249,38 +246,50 @@ try
} while (neededIndex >= limit);
}
if (!wasWrapForced)
assert(start >= 0);
// If we have already calculated the total length we can also assert that the limit is in range.
assert(ut->p == nullptr || static_cast<size_t>(limit) <= accessLength(ut));
// Even if we went out-of-bounds, we still need to update the chunkContents to contain the first/last chunk.
if (limit != limitOld)
{
const auto newSize = text.size() + 1;
const auto buffer = RefcountBuffer::EnsureCapacityForOverwrite(accessBuffer(ut), newSize);
if (!wasWrapForced)
{
const auto newSize = text.size() + 1;
const auto buffer = RefcountBuffer::EnsureCapacityForOverwrite(accessBuffer(ut), newSize);
memcpy(&buffer->data[0], text.data(), text.size() * sizeof(wchar_t));
til::at(buffer->data, text.size()) = L'\n';
memcpy(&buffer->data[0], text.data(), text.size() * sizeof(wchar_t));
til::at(buffer->data, text.size()) = L'\n';
text = { &buffer->data[0], newSize };
accessBuffer(ut) = buffer;
}
text = { &buffer->data[0], newSize };
accessBuffer(ut) = buffer;
}
accessCurrentRow(ut) = y;
ut->chunkNativeStart = start;
ut->chunkNativeLimit = limit;
ut->chunkLength = gsl::narrow_cast<int32_t>(text.size());
accessCurrentRow(ut) = y;
ut->chunkNativeStart = start;
ut->chunkNativeLimit = limit;
ut->chunkLength = gsl::narrow_cast<int32_t>(text.size());
#pragma warning(suppress : 26490) // Don't use reinterpret_cast (type.1).
ut->chunkContents = reinterpret_cast<const char16_t*>(text.data());
ut->nativeIndexingLimit = ut->chunkLength;
ut->chunkContents = reinterpret_cast<const char16_t*>(text.data());
ut->nativeIndexingLimit = ut->chunkLength;
}
}
auto offset = gsl::narrow_cast<int32_t>(nativeIndex - start);
// The ICU documentation is a little bit misleading. It states:
// > @param forward [...] If true, start<=index<limit. If false, [...] start<index<=limit.
// but that's just for finding the target chunk. The chunkOffset is not actually constrained to that!
// std::clamp will perform a<=b<=c, which is what we want.
const auto clampedIndex = std::clamp(nativeIndex, start, limit);
auto offset = gsl::narrow_cast<int32_t>(clampedIndex - start);
// Don't leave the offset on a trailing surrogate pair. See U16_SET_CP_START.
// This assumes that the TextBuffer contains valid UTF-16 which may theoretically not be the case.
if (offset > 0 && offset < ut->chunkLength && U16_IS_TRAIL(til::at(ut->chunkContents, offset)))
{
offset--;
}
ut->chunkOffset = offset;
return true;
return neededIndex >= start && neededIndex < limit;
}
catch (...)
{

8
src/cascadia/TerminalCore/Terminal.cpp
View File

@ -497,6 +497,10 @@ std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos)
result = GetHyperlinkIntervalFromViewportPosition(viewportPos);
if (result.has_value())
{
// GetPlainText and _ConvertToBufferCell work with inclusive coordinates, but interval's
// stop point is (horizontally) exclusive, so let's just update it.
result->stop.x--;
result->start = _ConvertToBufferCell(result->start);
result->stop = _ConvertToBufferCell(result->stop);
}
@ -522,10 +526,6 @@ std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos)
// Case 2 - Step 2: get the auto-detected hyperlink
if (result.has_value() && result->value == _hyperlinkPatternId)
{
// GetPlainText works with inclusive coordinates, but interval's stop
// point is (horizontally) exclusive, so let's just update it.
result->stop.x--;
return _activeBuffer().GetPlainText(result->start, result->stop);
}
return {};