-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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.
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
Sanitize C1 control chars in SetConsoleTitle API #10847
Conversation
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentSPACEBAR UnregisterTo 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:j4james/terminal.git repository
✏️ Contributor please read thisBy 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.
If the listed items are:
See the 🔬 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 garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a 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 stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
src/host/getset.cpp
Outdated
for (size_t i = 0; i < title.size(); i++) | ||
{ | ||
if (title.at(i) >= UNICODE_SPACE) | ||
const auto ch = title.at(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you convert this to a for (const auto ch : title)
loop?
On top of that I'd personally do it this way - it reduces binary size by half (but is unrelated to your PR):
std::wstring sanitized{title};
sanitized.erase(std::remove_if(sanitized.begin(), sanitized.end(), [](auto ch) -> bool {
return ch < 0x20 || (ch > 0x7f && ch < 0xa0);
}), sanitized.end());
return sanitized;
push_back
and emplace_back
are very expensive once they're inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with James keeping the code the way that it was (it's not his mess to clean up!), but if he wants to fix it all the more power to him. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like that approach. Not that thrilled with the code health bot's formatting, but at least it's more efficient now. Thanks @lhecker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this range stuff gets a bit neater with C++20, but I'm assuming we're not using that yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this range stuff gets a bit neater with C++20, but I'm assuming we're not using that yet?
alas no, we can't use c++20 in the OS build system quite yet
src/host/getset.cpp
Outdated
for (size_t i = 0; i < title.size(); i++) | ||
{ | ||
if (title.at(i) >= UNICODE_SPACE) | ||
const auto ch = title.at(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with James keeping the code the way that it was (it's not his mess to clean up!), but if he wants to fix it all the more power to him. 😄
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
When the `SetContoleTitle` API is called with a title containing control characters, we need to filter out those characters before we can forward the title change over conpty as an escape sequence. If we don't do that, the receiving terminal will end up executing the control characters instead of updating the title. We were already filtering out the C0 control characters, but with this PR we're now filtering out C1 controls characters as well. I've simply updated the sanitizing routine in `DoSrvSetConsoleTitleW` to filter our characters in the range `0x80` to `0x9F`. This is in addition to the C0 range (`0x00` to `0x1F`) that was already excluded. ## Validation Steps Performed I've added a conpty unit test that calls `DoSrvSetConsoleTitleW` with titles containing a variety of C0 and C1 controls characters, and which verifies that those characters are stripped from the title forwarded to conpty. I've also confirmed that the test case in issue #10312 is now working correctly in Windows Terminal. Closes #10312
🎉 Handy links: |
🎉 Handy links: |
When the
SetContoleTitle
API is called with a title containing controlcharacters, we need to filter out those characters before we can forward
the title change over conpty as an escape sequence. If we don't do that,
the receiving terminal will end up executing the control characters
instead of updating the title. We were already filtering out the C0
control characters, but with this PR we're now filtering out C1 controls
characters as well.
I've simply updated the sanitizing routine in
DoSrvSetConsoleTitleW
tofilter our characters in the range
0x80
to0x9F
. This is in additionto the C0 range (
0x00
to0x1F
) that was already excluded.Validation Steps Performed
I've added a conpty unit test that calls
DoSrvSetConsoleTitleW
withtitles containing a variety of C0 and C1 controls characters, and which
verifies that those characters are stripped from the title forwarded to
conpty.
I've also confirmed that the test case in issue #10312 is now working
correctly in Windows Terminal.
Closes #10312