Skip to content

Commit

Permalink
Fix DBCS attribute corruption during reflow (#12853)
Browse files Browse the repository at this point in the history 
855e136 contains a regression which breaks buffer reflow if wide surrogate
characters are present. This happens because we made use of the
`TextBufferCellIterator` whose increment operator skips 2 cells for wide
characters. This created a "misalignment" in the reflow logic which was written
for cell-wise iteration. This commit fixes the issue, by reverting back to the
previous algorithm without iterators.

Closes #12837
Closes MSFT-38904421

## Validation Steps Performed
* Run ``pwsh -noprofile -command echo "`u{D83D}`u{DE43}"``
* Resizing conhost preserves all contents ✅
* Resizing Windows Terminal doesn't crash it ✅
* Added a test covering this issue ✅

(cherry picked from commit 10b9044)
Service-Card-Id: 80340089
Service-Version: 1.12
  • Loading branch information
@lhecker @DHowett
lhecker authored and DHowett committed Apr 8, 2022
1 parent 36ba83b commit cd57f2c
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 17 deletions.
4 changes: 2 additions & 2 deletions src/buffer/out/OutputCellIterator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,14 @@ OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text) :
// Arguments:
// - utf16Text - UTF-16 text range
// - attribute - Color to apply over the entire range
OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute attribute) :
OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute& attribute, const size_t fillLimit) :
_mode(Mode::Loose),
_currentView(s_GenerateView(utf16Text, attribute)),
_run(utf16Text),
_attr(attribute),
_distance(0),
_pos(0),
_fillLimit(0)
_fillLimit(fillLimit)
{
}

Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/OutputCellIterator.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class OutputCellIterator final
OutputCellIterator(const wchar_t& wch, const TextAttribute& attr, const size_t fillLimit = 0) noexcept;
OutputCellIterator(const CHAR_INFO& charInfo, const size_t fillLimit = 0) noexcept;
OutputCellIterator(const std::wstring_view utf16Text);
OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute attribute);
OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute& attribute, const size_t fillLimit = 0);
OutputCellIterator(const gsl::span<const WORD> legacyAttributes) noexcept;
OutputCellIterator(const gsl::span<const CHAR_INFO> charInfos) noexcept;
OutputCellIterator(const gsl::span<const OutputCell> cells);
Expand Down
18 changes: 5 additions & 13 deletions src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2245,8 +2245,6 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
// the "right" boundary, which is one past the final valid
// character)
short iOldCol = 0;
auto chars{ row.GetCharRow().cbegin() };
auto attrs{ row.GetAttrRow().begin() };
const auto copyRight = iRight;
for (; iOldCol < copyRight; iOldCol++)
{
Expand All @@ -2259,9 +2257,9 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
try
{
// TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
const auto glyph = chars->Char();
const auto dbcsAttr = chars->DbcsAttr();
const auto textAttr = *attrs;
const auto glyph = row.GetCharRow().GlyphAt(iOldCol);
const auto dbcsAttr = row.GetCharRow().DbcsAttrAt(iOldCol);
const auto textAttr = row.GetAttrRow().GetAttrByColumn(iOldCol);

if (!newBuffer.InsertCharacter(glyph, dbcsAttr, textAttr))
{
Expand All @@ -2270,9 +2268,6 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
}
}
CATCH_RETURN();

++chars;
++attrs;
}

// GH#32: Copy the attributes from the rest of the row into this new buffer.
Expand Down Expand Up @@ -2303,21 +2298,18 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
// for inserting an attr would be past the right of the new buffer.
for (short copyAttrCol = iOldCol;
copyAttrCol < cOldColsTotal && newAttrColumn < newWidth;
copyAttrCol++)
copyAttrCol++, newAttrColumn++)
{
try
{
// TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
const auto textAttr = *attrs;
const auto textAttr = row.GetAttrRow().GetAttrByColumn(copyAttrCol);
if (!newRow.GetAttrRow().SetAttrToEnd(newAttrColumn, textAttr))
{
break;
}
}
CATCH_LOG(); // Not worth dying over.

++newAttrColumn;
++attrs;
}

// If we found the old row that the caller was interested in, set the
Expand Down
7 changes: 6 additions & 1 deletion src/host/ut_host/ScreenBufferTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6296,6 +6296,7 @@ void ScreenBufferTests::TestReflowEndOfLineColor()
stateMachine.ProcessString(L"\x1b[44m"); // Blue BG
stateMachine.ProcessString(L" CCC \n"); // " abc " (with spaces on either side)
stateMachine.ProcessString(L"\x1b[43m"); // yellow BG
stateMachine.ProcessString(L"\U0001F643\n"); // GH#12837: Support for surrogate characters during reflow
stateMachine.ProcessString(L"\x1b[K"); // Erase line
stateMachine.ProcessString(L"\x1b[2;6H"); // move the cursor to the end of the BBBBB's

Expand All @@ -6317,7 +6318,11 @@ void ScreenBufferTests::TestReflowEndOfLineColor()
TestUtils::VerifyLineContains(iter2, L' ', defaultAttrs, static_cast<size_t>(width - 5));

auto iter3 = tb.GetCellLineDataAt({ 0, 3 });
TestUtils::VerifyLineContains(iter3, L' ', yellow, static_cast<size_t>(width));
TestUtils::VerifyLineContains(iter3, L"\U0001F643", yellow, 2u);
TestUtils::VerifyLineContains(iter3, L' ', defaultAttrs, static_cast<size_t>(width - 2));

auto iter4 = tb.GetCellLineDataAt({ 0, 4 });
TestUtils::VerifyLineContains(iter4, L' ', yellow, static_cast<size_t>(width));
};

Log::Comment(L"========== Checking the buffer state (before) ==========");
Expand Down

1 comment on commit cd57f2c

@github-actions
Copy link

@github-actions github-actions bot commented on cd57f2c Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@check-spelling-bot Report

Unrecognized words, please review:

  • hwinsta
  • Lsa
  • lsass
  • ofstream
  • tmpdir
  • UOI
  • USEROBJECTFLAGS
  • winsta
  • winstamin
  • WSF
Previously acknowledged words that are now absent adaa carlos coffgroup coffgrp datetime DCompile deconstructed devicefamily dpg eae emplate GENPROFILE GTR guardxfg HHmm Hostx installationpath MMdd MSDL pgorepro pgort PGU redistributable sid SPACEBAR Timeline timelines Unregister UWA UWAs vcvarsall xfg xIcon yIcon zamora
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the release-1.12 branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/cd57f2c24e2852cd6094acf2159ff2a867936c59.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/comments/70832363" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

Please sign in to comment.