Fix a OOB write in SixelParser#20153
Conversation
|
Would it not be simpler to catch the out of memory exception in the handler here, and then return false? terminal/src/terminal/adapter/SixelParser.cpp Lines 92 to 95 in eeb9f7a I believe that should tell the state machine to ignore all further content in the sequence, so there should be no chance of the buffer being used after that. And while dealing with the OOM exception is a good thing either way, I'd prefer it if you also implemented the patch I posted in #20149 (comment), because this particular case should never have used that much memory in the first place - that's the real bug that #20149 identified. If you don't want to address that in this PR, at least leave the issue open. |
omg I completely forgot about this behavior of our parser! I wonder why this didn't occur to me...
I intended to file a follow-up issue. But given that your suggested fix is a lot tidier than mine, I'll simply piggy-back your proper fix here. Thank you for the feedback! |
…20213) This commit implements two fixes for the integer overflow/out-of-bounds write reported in #20149. First, it catches any exception generated in sixel char processing (which will prevent `out_of_memory` or `bad_alloc` from being ignored sight-unseen, and will prevent the consumption of further DCS content). Second, it prevents us from allocating memory for an image which will never be displayed (because it exceeds the height of the display.) This supersedes prior work in #20153 for the same issues. Closes #20149 Closes #20153 Co-authored-by: James Holderness <j4_james@hotmail.com> (cherry picked from commit c829d4c) Service-Card-Id: PVTI_lADOAF3p4s4BBcTlzgrY_zM Service-Version: 1.24
…20213) This commit implements two fixes for the integer overflow/out-of-bounds write reported in #20149. First, it catches any exception generated in sixel char processing (which will prevent `out_of_memory` or `bad_alloc` from being ignored sight-unseen, and will prevent the consumption of further DCS content). Second, it prevents us from allocating memory for an image which will never be displayed (because it exceeds the height of the display.) This supersedes prior work in #20153 for the same issues. Closes #20149 Closes #20153 Co-authored-by: James Holderness <j4_james@hotmail.com> (cherry picked from commit c829d4c) Service-Card-Id: PVTI_lADOAF3p4s4BQX0-zgrY_zQ Service-Version: 1.25
The
SixelParserstate machine assumes that_resizeImageBuffersucceeded. The state that determines the X/Y size of the buffer
is set outside of the resize function.
This meant that an OOM failure can cause the resize to fail
and the buffer X/Y size variables to be out-of-sync.
A later write in the buffer will then be out of bounds.
Whether that's before or after the buffer address depends
on whether the 32-bit coord multiplication overflows or not.
Closes #20149
Validation Steps Performed
in PowerShell 5 (depends on its particular chunking)