Questionable use of cur_comp_info in JpegImageDecoder |
||
Issue descriptionThe if statement in [1] is suspicious. We check that info.num_components == 3. This is the number of components in the JPEG. Then, we try to access the first three elements in info.cur_comp_info. However, info.num_components is not related to info.cur_comp_info. The former is the number of components in the whole image. The latter are the components in the current scan. So, if the image has 3 components in total (e.g., YCbCr), but the current scan only has one component (e.g., Y), we are going out of bounds. As a note: YuvSubsampling is called after jpeg_read_header(). According to the documentation for the latter, "this routine will read as far as the first SOS marker." So, if I'm interpreting that correctly, it means that YuvSubsampling is only concerned with the first scan. scroggo@ CCing you because I don't know if the component I chose is the most appropriate. [1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/image-decoders/jpeg/jpeg_image_decoder.cc?l=285-290&rcl=09928a3d8a901d72298ded4f8206c325bcbbf637
,
Nov 2
+jvanverth@, who has been looking at using YUV > scroggo@ CCing you because I don't know if the component I chose is the > most appropriate. Oh, hahaha - you meant the Chromium Component (thanks vmiura@). The rest of the bug is talking about jpeg file components, so I was thinking about that :P From your comments in https://chromium-review.googlesource.com/c/chromium/src/+/1235214/1/third_party/blink/renderer/platform/image-decoders/jpeg/jpeg_image_decoder.cc#147 , it indeed sounds like YUV is doing something wrong. Your comment referenced issue 598670 . That bug also raises a concern about using cur_comp_info. Oddly, in its fix (https://codereview.chromium.org/1849393002/), it sticks with cur_comp_info (for YuvSubsampling; outputRawData gets changed), but it removes the check for info.comps_in_scan being large enough. The bug and the fix have some instructions for how to repro, and there are even layout tests. But it's not obvious there was any effort to test images like you suggest, where we may go out of bounds. But I don't think there's an issue of reading out of bounds. cur_comp_info is defined as jpeg_component_info *cur_comp_info[MAX_COMPS_IN_SCAN]; [2] So it's an array of pointers. There are four (MAX_COMPS_IN_SCAN) pointers jammed in the struct. So cur_comp_info is never null, and your link in [1] checks each pointer before dereferencing it. So while I agree that there may be a bug here, I don't think we're doing any out of bounds reads. [2] https://chromium.googlesource.com/chromium/deps/libjpeg_turbo.git/+/0d47d2d3a728e78676a15b1d818cc668cb7e5a9c/jpeglib.h#438 |
||
►
Sign in to add a comment |
||
Comment 1 by vmi...@chromium.org
, Nov 2Status: Available (was: Unconfirmed)