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

Jump to bottom

TextBuffer::ResizeTraditional may leak stale pointers #14696

Closed
lhecker opened this issue Jan 18, 2023 · 2 comments · Fixed by #15105
Closed

TextBuffer::ResizeTraditional may leak stale pointers #14696

lhecker opened this issue Jan 18, 2023 · 2 comments · Fixed by #15105
Assignees
@lhecker
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Milestone
Terminal v1.18

Comments

@lhecker
Copy link
Member

lhecker commented Jan 18, 2023

The catch clause will allow BufferAllocator to be destroyed while the ROWs in _storage continue to reference its memory. The solution is to refactor ROW::Resize from being a mutating function over to allocating a new _storage vector and copying all rows over. That way the TextBuffer is only mutated once at the end, long after all throwing code has finished executing.
@lhecker lhecker added Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-3 A description (P3) labels Jan 18, 2023
@lhecker lhecker self-assigned this Jan 18, 2023
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 18, 2023
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 18, 2023
@carlos-zamora carlos-zamora added this to the Terminal v1.18 milestone Jan 18, 2023
@lhecker lhecker mentioned this issue Jan 18, 2023
Merged
@zadjii-msft
Copy link
Member

@lhecker was this closed in #15105?

@lhecker
Copy link
Member Author

lhecker commented Apr 4, 2023

Oh, yes, thank you!

@lhecker lhecker mentioned this issue Apr 4, 2023
Merged
zadjii-msft pushed a commit that referenced this issue Apr 5, 2023
@lhecker
Use new row primitives for ResizeTraditional (#15105)
ecb5e37 
This will allow us to share the same fundamental text insertion
logic for both `ResizeTraditional` and `Reflow`, because both
can be implemented with `ROW::CopyRangeFrom`. It also replaces
the `BufferAllocator` struct with a `_allocateBuffer` function
which will help us allocate scratch buffer rows in the future.

Closes #14696

## PR Checklist
* Disable reflow resize in conhost
* Print "zhwik8.txt" - a enwik8.txt equivalent of Chinese Wikipedia
* Run `color 80` in cmd
* Resize windows from 120 to 119 columns
* Wide glyphs disappear and are replaced with whitespace ✅
* Resizing the window to >120 columns adds gray whitespace ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Tag-Fix Doesn't match tag requirements In-PR This issue has a related PR labels Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Terminal v1.18
Development

Successfully merging a pull request may close this issue.

Use new row primitives for ResizeTraditional microsoft/terminal
3 participants
@lhecker @carlos-zamora @zadjii-msft