[Bug 48927] New: Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image
https://bugs.winehq.org/show_bug.cgi?id=48927 Bug ID: 48927 Summary: Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image Product: Wine-staging Version: unspecified Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs(a)winehq.org Reporter: thomas.faber(a)reactos.org CC: leslie_alistair(a)hotmail.com, z.figura12(a)gmail.com Distribution: --- Created attachment 66887 --> https://bugs.winehq.org/attachment.cgi?id=66887 Debugger info from ReactOS ReactOS bug for reference: https://jira.reactos.org/browse/CORE-16796 Apologies for not reproducing this on Wine; the bug & fix are pretty simple though. The gdiplus:image test tries to decode a 1x1 TIFF image, and TiffFrameDecode_ReadTile assumes that the cached_tile is large enough for an even number of output pixels (i.e. a full number of input bytes). The issue appears to be with this Staging patch: https://github.com/wine-staging/wine-staging/blob/master/patches/windowscode... The attachment has a backtrace and relevant variables. The line numbers may not match but the underflow got caught at: dst[0] = (b & 0x20) ? 0xff : 0; /* B */ -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 --- Comment #1 from Thomas Faber <thomas.faber(a)reactos.org> --- Created attachment 66888 --> https://bugs.winehq.org/attachment.cgi?id=66888 Possible fix Attaching a possible fix (line numbers may not match but context should). I'm not sure of the proper process for updating someone else's patch in Staging. Happy to follow it if preferred, or someone can just fix it directly. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever confirmed|0 |1 --- Comment #2 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- (In reply to Thomas Faber from comment #1)
Created attachment 66888 [details] Possible fix
Attaching a possible fix (line numbers may not match but context should).
I'm not sure of the proper process for updating someone else's patch in Staging. Happy to follow it if preferred, or someone can just fix it directly.
While the compiler usually should be able to optimize this code it's always preferable to use '& 1' instead of '%2'. Does using simple if (!(This->decode_info.tile_width & 1)) help? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 --- Comment #3 from Thomas Faber <thomas.faber(a)reactos.org> --- In all but the last iteration, two bytes should be written (or we'd skip every other pixel) -- so we'd still need the "count ||", right? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 --- Comment #4 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- I don't have a TIFF image in an appropriate format to test at the moment, however does the following implementation fix the heap corruption? /* 1 source byte expands to 2 BGRA samples */ count = This->decode_info.tile_width * This->decode_info.tile_height; src = This->cached_tile + count / 2 - 1; dst = This->cached_tile + This->decode_info.tile_size; while (count >= 2) { BYTE b = *src--; dst -= 8; dst[2] = (b & 0x80) ? 0xff : 0; /* R */ dst[1] = (b & 0x40) ? 0xff : 0; /* G */ dst[0] = (b & 0x20) ? 0xff : 0; /* B */ dst[3] = (b & 0x10) ? 0xff : 0; /* A */ dst[6] = (b & 0x08) ? 0xff : 0; /* R */ dst[5] = (b & 0x04) ? 0xff : 0; /* G */ dst[4] = (b & 0x02) ? 0xff : 0; /* B */ dst[7] = (b & 0x01) ? 0xff : 0; /* A */ count -= 2; } if (count) { BYTE b = *src--; dst -= 4; dst[2] = (b & 0x80) ? 0xff : 0; /* R */ dst[1] = (b & 0x40) ? 0xff : 0; /* G */ dst[0] = (b & 0x20) ? 0xff : 0; /* B */ dst[3] = (b & 0x10) ? 0xff : 0; /* A */ } -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 --- Comment #5 from Thomas Faber <thomas.faber(a)reactos.org> --- Almost! Because this no longer rounds up "count" before dividing by two, "src" ends up one byte before "cached_tile" here: src = This->cached_tile + count / 2 - 1; I fixed this to be: src = This->cached_tile + (count + 1) / 2 - 1; With that modification, there's no more out of bounds access. There may be other complications to consider at some point, such as whether byte-alignment needs to happen at the end of each row (so the bottom half of the last byte of each row would be skipped). But for the 1x1 case this seems to work great now. I've tested this using the gdiplus:image test. Thanks! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 --- Comment #6 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- (In reply to Thomas Faber from comment #5)
Almost!
Because this no longer rounds up "count" before dividing by two, "src" ends up one byte before "cached_tile" here: src = This->cached_tile + count / 2 - 1;
I fixed this to be: src = This->cached_tile + (count + 1) / 2 - 1;
With that modification, there's no more out of bounds access.
Thanks for testing.
There may be other complications to consider at some point, such as whether byte-alignment needs to happen at the end of each row (so the bottom half of the last byte of each row would be skipped). But for the 1x1 case this seems to work great now. I've tested this using the gdiplus:image test.
That would need a real test case. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 --- Comment #7 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- Created attachment 66946 --> https://bugs.winehq.org/attachment.cgi?id=66946 WIP patch Just in case I'm attaching a patch against staged 0012-windowscodecs-Add-support-for-4bpp-RGBA-format-to-TI.patch -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66946|0 |1 is patch| | -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |5.6 --- Comment #8 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- I've sent another version of the fix that doesn't avoid an intermediate memory allocation: https://source.winehq.org/patches/data/183871 along with a specific test for the 4bps BGRA TIFF: https://source.winehq.org/patches/data/183867 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|NEW |RESOLVED Fixed by SHA1| |0cd8502b493e7a93df7aad08bc2 | |fdfe3fd73b5cc --- Comment #9 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- (In reply to Dmitry Timoshkov from comment #8)
I've sent another version of the fix that doesn't avoid an intermediate memory allocation: https://source.winehq.org/patches/data/183871 along with a specific test for the 4bps BGRA TIFF: https://source.winehq.org/patches/data/183867
0cd8502b493e7a93df7aad08bc2fdfe3fd73b5cc contains new implementation. Note: it's a winehq commit. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|-unknown |windowscodecs Product|Wine-staging |Wine --- Comment #10 from Zebediah Figura <z.figura12(a)gmail.com> --- (In reply to Dmitry Timoshkov from comment #9)
(In reply to Dmitry Timoshkov from comment #8)
I've sent another version of the fix that doesn't avoid an intermediate memory allocation: https://source.winehq.org/patches/data/183871 along with a specific test for the 4bps BGRA TIFF: https://source.winehq.org/patches/data/183867
0cd8502b493e7a93df7aad08bc2fdfe3fd73b5cc contains new implementation.
Note: it's a winehq commit.
Never quite sure how to reasonably deal with these, but I'm going to move the bug to the Wine product in that case. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48927 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #11 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 5.8. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla