Invalid member values from opj_read_header or opj_decode ? #411

gcode-importer · 2014-10-09T22:10:42Z

Originally reported on Google Code with ID 411

https://code.google.com/p/chromium/issues/detail?id=418881

Comment from Bo :
@antonin, can you take a look at this one. The crash is not inside jp2, but it looks
like the image from opj_read_header or opj_decode has invalid member values.

Reported by detonin on 2014-10-09 22:10:42

The text was updated successfully, but these errors were encountered:

gcode-importer · 2014-10-09T22:12:23Z

@Bo: could you cc m.darbois on the chromium side so that he can access the issue in
case he has a look to it before I do ? Thks

Reported by detonin on 2014-10-09 22:12:23

gcode-importer · 2014-10-09T22:18:01Z

I just added m.darbois. thanks.

Reported by bo_xu@foxitsoftware.com on 2014-10-09 22:18:01

gcode-importer · 2014-10-10T17:07:00Z

@bo_xu,

I guess you reused sycc422_to_rgb from OpenJpeg in your file fx_codec_jpx_opj.cpp

The following patch corrects this function when width is odd.

@antonin,
I had to modify opj_decompress in order to follow that path (image is reported as sRGB
by colr box). Not sure you want to apply (not tested against test suite). At least
present for reference.

BEFORE issue411.patch
./bin/opj_decompress -i ../../data/issue411/0.jp2 -o 0.bmp

[INFO] Start to read j2k main header (85).
[INFO] Main header has been correctly decoded.
[INFO] No decoded area parameters, set the decoded area to the whole image
[INFO] Psot value of the current tile-part is equal to zero, we assuming it is the
last tile-part of the codestream.
[INFO] Header of tile 1 / 1 has been read.
[INFO] Tile 1/1 has been decoded.
[INFO] Image data has been updated with tile 1.

[INFO] Stream reached its end !
=================================================================
==14579==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x03f25700 at pc
0x00034c0a bp 0xbffeb688 sp 0xbffeb684
READ of size 4 at 0x03f25700 thread T0
    #0 0x34c09 in sycc422_to_rgb (/Users/Matt/Dev/OpenJpeg/issue391/build/./bin/opj_decompress+0x23c09)
    #1 0x33777 in color_sycc_to_rgb (/Users/Matt/Dev/OpenJpeg/issue391/build/./bin/opj_decompress+0x22777)
    #2 0x17a90 in main (/Users/Matt/Dev/OpenJpeg/issue391/build/./bin/opj_decompress+0x6a90)
    #3 0x9aaf1700 in start (/usr/lib/system/libdyld.dylib+0x3700)
    #4 0x4 (<unknown module>)

0x03f25700 is located 0 bytes to the right of 1195776-byte region [0x03e01800,0x03f25700)
allocated by thread T0 here:
    #0 0x27e30a in wrap_calloc (/Users/Matt/Dev/llvm-clang-3.5.0-macosx-apple-darwin/lib/clang/3.5.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x3030a)
    #1 0x76ee59 in opj_j2k_update_image_data /Users/Matt/Dev/OpenJpeg/issue391/src/lib/openjp2/j2k.c:9482:31
    #2 0x76fe31 in opj_j2k_decode_tiles /Users/Matt/Dev/OpenJpeg/issue391/src/lib/openjp2/j2k.c:9326:21
    #3 0x757547 in opj_j2k_exec /Users/Matt/Dev/OpenJpeg/issue391/src/lib/openjp2/j2k.c:6718:9
    #4 0x761033 in opj_j2k_decode /Users/Matt/Dev/OpenJpeg/issue391/src/lib/openjp2/j2k.c:9170:9
    #5 0x77737f in opj_jp2_decode /Users/Matt/Dev/OpenJpeg/issue391/src/lib/openjp2/jp2.c:1192:6
    #6 0x782873 in opj_decode /Users/Matt/Dev/OpenJpeg/issue391/src/lib/openjp2/openjpeg.c:370:3
    #7 0x17856 in main (/Users/Matt/Dev/OpenJpeg/issue391/build/./bin/opj_decompress+0x6856)
    #8 0x9aaf1700 in start (/usr/lib/system/libdyld.dylib+0x3700)
    #9 0x4 (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 sycc422_to_rgb
Shadow bytes around the buggy address:
  0x207e4a90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x207e4aa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x207e4ab0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x207e4ac0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x207e4ad0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x207e4ae0:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x207e4af0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x207e4b00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x207e4b10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x207e4b20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x207e4b30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  ASan internal:           fe
==14579==ABORTING

AFTER issue411.patch :
./bin/opj_decompress -i ../../data/issue411/0.jp2 -o 0.bmp

[INFO] Start to read j2k main header (85).
[INFO] Main header has been correctly decoded.
[INFO] No decoded area parameters, set the decoded area to the whole image
[INFO] Psot value of the current tile-part is equal to zero, we assuming it is the
last tile-part of the codestream.
[INFO] Header of tile 1 / 1 has been read.
[INFO] Tile 1/1 has been decoded.
[INFO] Image data has been updated with tile 1.

[INFO] Stream reached its end !
[INFO] Generated Outfile 0.bmp

Reported by mayeut on 2014-10-10 17:07:00

Status changed: Verified

- _Attachment: [0.jp2](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-411/comment-3/0.jp2)_ - _Attachment: [opj_decompress.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-411/comment-3/opj_decompress.patch)_

gcode-importer · 2014-10-10T17:11:28Z

@antonin,

I only fixed sycc422_to_rgb which I can test.
This "odd" bug will most probably occur in other sycc_* with subsampling

Reported by mayeut on 2014-10-10 17:11:28

gcode-importer · 2014-10-10T18:22:41Z

Thanks guys, when do you think can add this to the trunk? I will just do a update to
resolve this issue.

Reported by bo_xu@foxitsoftware.com on 2014-10-10 18:22:41

gcode-importer · 2014-10-10T18:43:11Z

@bo_xu,

As I said in comment #3, the commit will probably not solve your issue unless fx_codec_jpx_opj.cpp
is generated automatically using src/bin/common/color.c

If it's not generated automatically, you can already "apply" the patch on your end.

Reported by mayeut on 2014-10-10 18:43:11

gcode-importer · 2014-10-10T19:13:23Z

Thanks, now I see. 

So I guess I need to do similar changes to other sycc*** functions. If you have plan
to do it (I guess in color.c), I may want to follow that and add to PDFium, since I
am not 100% sure if I can add others the right way.

Reported by bo_xu@foxitsoftware.com on 2014-10-10 19:13:23

gcode-importer · 2014-10-10T21:58:08Z

Maybe the for loop could be written in the following way?

for (j = 0; j < maxw; ++j, ++y, ++r, ++g, ++b) {
  sycc_to_rgb(offset, upb, *y, *cb, *cr, r, g, b);
  if (j % 2) {
     ++cr;
     ++cb;
  }
}

Reported by bo_xu@foxitsoftware.com on 2014-10-10 21:58:08

gcode-importer · 2014-10-10T23:17:39Z

@bo_xu,

Previous file was actually buggy...
You need ++cr, ++cb on the last pixel even if it's odd (means j%2 won't work).

Verified against Asan, still not complaining, means it's right.

Reported by mayeut on 2014-10-10 23:17:39

- _Attachment: [issue411.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-411/comment-9/issue411.patch)_ - _Attachment: [color.c](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-411/comment-9/color.c)_

gcode-importer · 2014-10-10T23:39:32Z

I see, guess I might just change #8 to " if(j%2 || j == maxh-1) " ?

Reported by bo_xu@foxitsoftware.com on 2014-10-10 23:39:32

gcode-importer · 2014-10-11T00:04:34Z

you may but it will be less efficient than proposed implementation (unless the compiler
optimizes these tests away by unrolling the loop itself)

Reported by mayeut on 2014-10-11 00:04:34

gcode-importer · 2014-10-11T00:11:01Z

I won't think that will affect the efficiency much, if any. And this way may make the
other part of the patch more clear and easy to understand:

    for(i = 0; i < maxh; i++) {
        if (i % 2) {
            ny = y + maxw; nr = r + maxw; ng = g + maxw; nb = b + maxw;
            for (j = 0; j < maxw; j++) {
                sycc_to_rgb(offset, upb, *y, *cb, *cr, r, g, b);
                ++y;  ++r;  ++g;  ++b;
                sycc_to_rgb(offset, upb, *ny, *cb, *cr, nr, ng, nb);
                ++ny;  ++nr;  ++ng;  ++nb;
                if (j % 2 || j == maxw - 1){
                    ++cb;  ++cr;
                }
            }
            y += maxw; r += maxw;
            g += maxw;
            b += maxw;
        }
        if (i == maxh - 1 && i % 2 == 0){
            for (j = 0; j < maxw; j++) {
                sycc_to_rgb(offset, upb, *y, *cb, *cr, r, g, b);
                ++y;  ++r;  ++g;  ++b;
                sycc_to_rgb(offset, upb, *ny, *cb, *cr, nr, ng, nb);
                ++ny;  ++nr;  ++ng;  ++nb;
                if (j % 2 || j == maxw - 1){
                    ++cb;  ++cr;
                }
            }
        }
    }

Reported by bo_xu@foxitsoftware.com on 2014-10-11 00:11:01

gcode-importer · 2014-10-11T00:12:59Z

Just to confirm, in color.c of #9, after line 248, do we miss 

++y; ++r; ++g; ++b; ++cb; ++cr; 

?

Reported by bo_xu@foxitsoftware.com on 2014-10-11 00:12:59

gcode-importer · 2014-10-11T10:01:27Z

It's outside of the loop & not used afterwards so no.

IMHO,
I think you'll find many conversion functions using the same patterns as OpenJpeg in
the rest of the world rather than modulo trick.
Thus, it's much clearer for someone used to image processing to read OpenJpeg code
rather than what you're proposing.

Reported by mayeut on 2014-10-11 10:01:27

gcode-importer · 2014-10-11T18:31:49Z

Fair enough :)

Reported by bo_xu@foxitsoftware.com on 2014-10-11 18:31:49

gcode-importer · 2014-10-21T15:22:25Z

This issue was closed by revision r2908.

Reported by detonin on 2014-10-21 15:22:25

Status changed: Fixed

gcode-importer added bug Priority-Critical Restrict-View-CoreTeam labels Jun 11, 2015

gcode-importer assigned detonin Jun 11, 2015

gcode-importer closed this as completed Jun 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalid member values from opj_read_header or opj_decode ? #411

Invalid member values from opj_read_header or opj_decode ? #411

gcode-importer commented Oct 9, 2014

gcode-importer commented Oct 9, 2014

gcode-importer commented Oct 9, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 21, 2014

Invalid member values from opj_read_header or opj_decode ? #411

Invalid member values from opj_read_header or opj_decode ? #411

Comments

gcode-importer commented Oct 9, 2014

gcode-importer commented Oct 9, 2014

gcode-importer commented Oct 9, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 10, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 11, 2014

gcode-importer commented Oct 21, 2014