Bug 690723

Summary: Using unallocated memory when parsing image...
Product: jbig2dec Reporter: Sebastian Rasmussen <sebastian.rasmussen>
Component: ParsingAssignee: Henry Stiles <henry.stiles>
Status: RESOLVED FIXED    
Severity: minor CC: masaki.ushizaka, shailesh.mistry
Priority: P4    
Version: 0.10   
Hardware: PC   
OS: Linux   
Customer: Word Size: ---
Attachments: globals.jbig2
img.jbig2
scan.pdf
Patch for Bug690723

Description Sebastian Rasmussen 2009-08-20 03:12:23 UTC 
I have stumbled upon a .pdf-file valgrind complained about when showing it in
MuPDF. According to valgrind it is due to reading outside an allocated buffer in
libjbig2dec as shown below, and the problem persists if I do valgrind jbig2dec
-t png globals.jbig2 img.jbig2. I attached both the image, globals and the
original pdf.

==6572== Invalid read of size 1
==6572==    at 0x40461D3: (within /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x4046583: jbig2_decode_generic_mmr (in
/usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x4041F98: jbig2_symbol_dictionary (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x403F6BE: jbig2_parse_segment (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x403E232: jbig2_data_in (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x8049598: (within /usr/bin/jbig2dec)
==6572==    by 0x40C0EBB: (below main) (libc-start.c:231)
==6572==  Address 0x42428D9 is 0 bytes after a block of size 345 alloc'd
==6572==    at 0x4021620: malloc (vg_replace_malloc.c:149)
==6572==    by 0x403DEAC: (within /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x403DA14: jbig2_alloc (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x4047ADA: jbig2_image_new (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x4041CAD: jbig2_symbol_dictionary (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x403F6BE: jbig2_parse_segment (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x403E232: jbig2_data_in (in /usr/lib/libjbig2dec.so.0.0.0)
==6572==    by 0x8049598: (within /usr/bin/jbig2dec)
==6572==    by 0x40C0EBB: (below main) (libc-start.c:231)
Comment 1 Sebastian Rasmussen 2009-08-20 03:13:11 UTC 
Created attachment 5317 [details]
globals.jbig2
Comment 2 Sebastian Rasmussen 2009-08-20 03:13:31 UTC 
Created attachment 5318 [details]
img.jbig2
Comment 3 Sebastian Rasmussen 2009-08-20 03:14:03 UTC 
Created attachment 5319 [details]
scan.pdf

... and this would be the original pdf-file.
Comment 4 Shailesh Mistry 2012-01-17 19:34:15 UTC 
Created attachment 8287 [details]
Patch for Bug690723

This bug only appears when the final colour change happens right at the width of the image and the width is divisible by 8. The patch looks for this specific condition and prevents the buffer read/write overrun.

The patch has been tested on the regression cluster and using valgrind without errors.
Comment 5 Henry Stiles 2012-02-08 16:34:37 UTC 
(In reply to comment #4)
> Created an attachment (id=8287) [details]
> Patch for Bug690723
> 
> This bug only appears when the final colour change happens right at the width
> of the image and the width is divisible by 8. The patch looks for this specific
> condition and prevents the buffer read/write overrun.
> 
> The patch has been tested on the regression cluster and using valgrind without
> errors.

I'm not quite clear on this one.  It looks to me like the original intent was a precondition that x1 and x0 would not overflow or underflow.  This is enforced by clamping to the width as in:


				a1 = a0 + white_run;
				a2 = a1 + black_run;
				if (a1 > mmr->width) a1 = mmr->width;
				if (a2 > mmr->width) a2 = mmr->width;
                                jbig2_set_bits(dst, a1, a2);


I didn't step through in the debugger but notice here:


			jbig2_decode_mmr_consume(mmr, 1);
			b1 = jbig2_find_changing_element_of_color(ref, a0, mmr->width, !c);
			if (c) jbig2_set_bits(dst, a0, b1);

we have the colour transition you talk about but no clamping b1 as above.

I don't know if that is the exact problem but I think it best to keep the precondition (x1 and x0 won't go out of bounds) and not add another parameter to address work around the precondition for a particular case.
Comment 6 Shailesh Mistry 2012-06-20 17:19:37 UTC 
The final patch used was 


--- a/gs/jbig2dec/jbig2_mmr.c
+++ b/gs/jbig2dec/jbig2_mmr.c
@@ -792,7 +792,7 @@ jbig2_set_bits(byte *line, int x0, int x1)
                line[a0] |= lm[b0];
                for (a = a0 + 1; a < a1; a++)
                        line[a] = 0xFF;
-               line[a1] |= rm[b1];
+               if (b1) line[a1] |= rm[b1];
        }
 }

This has been cluster tested and committed as dc77e931b2a6118092bac21b4dd38bc10d41e644
Comment 7 Henry Stiles 2012-09-26 05:56:37 UTC 
Fixed see Comment #6