Created attachment 9999 [details] log.txt Seg faults in the 64 bit build of mupdf were found by fuzzing in jbig2_image_compose (jbig2_image.c:292) while reading these files. See the attached log.txt for details. mupdf__3324.pdf.asan.50.2585.pgmraw.200.0
See bug 694111 for a fix (might be the same issue).
$100.00 bounty
I'm unable to reproduce this crash (under 32-bit Windows 7).
This does still occur on 64bit linux.
Created attachment 10427 [details] Patch jbig2dec in mupdf/thirdparty As requested, I tested this on 64 bit windows and Linux. The crash and valgrind errors have been resolved by the attached patch. This patch also addresses differences between the jbig2dec folder in gs with the one in mupdf/thirdparty.
Created attachment 10428 [details] Patch for gs/jbig2dec Difference were found between gs/jbig2dec and mupdf/thirdparty/jbig2dec. The attached patch addresses updates required to gs/jbig2dec to bring it inline.
(In reply to comment #6) > Difference were found between gs/jbig2dec and mupdf/thirdparty/jbig2dec. The > attached patch addresses updates required to gs/jbig2dec to bring it inline. Thanks for looking into the differences between gs and mupdf/thirdparty (shouldn't they be sync'ed automatically anyway?). You're fixing these at the wrong end, though: The first two changes would reintroduce a warning with MSVC (unreachable code) and with the third change, cleanup code would be skipped.
MuPDF now gets its jbig2dec from the standalone jbig2dec repo. This is generated automatically from the ghostscript copy daily - hence fixes should be applied to the ghostscript one, and then the MuPDF submodule should be updated. All of the first patches fixes were already in the gs and standalone jbig2decs, but the MuPDF submodule had not been moved on. I've pushed the final patch back into the gs one, and that's regenerated the standalone one. As soon as Tor reviews it, I will update the submodule and the fix should be pulled into MuPDF too. This does indeed seem to solve the problem. Thanks!
(In reply to comment #8) > I've pushed the final patch back into the gs one, and that's regenerated the > standalone one. Did you see comment #7? That patch reintroduces two bugs which were fixed with http://git.ghostscript.com/?p=jbig2dec.git;a=commitdiff;h=b009e994db066f204f85cb7935ebd5e7a0e1cb6c . Please back it out again.
Fixed with: commit 7bb202b88e012d57d73463b239dae82cbb7d457d Author: Robin Watts <robin@peeves.(none)> Date: Thu Nov 28 09:15:29 2013 -0800 Pull in the latest jbig2dec changes. These were the last few fixes that went back from mupdf to gs. This seems to solve bug 694362. Thanks to Shelly for this.
(In reply to comment #9) > (In reply to comment #8) > > I've pushed the final patch back into the gs one, and that's regenerated the > > standalone one. > > Did you see comment #7? That patch reintroduces two bugs which were fixed > with > http://git.ghostscript.com/?p=jbig2dec.git;a=commitdiff; > h=b009e994db066f204f85cb7935ebd5e7a0e1cb6c . Please back it out again. I believe that all your changes are properly in there now. I will double check again now. If I have missed something, please let me know.
Simon, you are right in that I have effectively backed out part of the commit you mention. I've put back 2 'return 0;' statements, which are indeed unreachable and will cause warnings. This was wrong of me, and I will take them out again. The other change is actually the key thing that solves this bug. In jbig2_symbol_dict.c your patch does: @@ -950,7 +953,7 @@ jbig2_symbol_dictionary(Jbig2Ctx *ctx, Jbig2Segment *segment, break; case 2: default: - return jbig2_error(ctx, JBIG2_SEVERITY_FATAL, segment->number, + jbig2_error(ctx, JBIG2_SEVERITY_FATAL, segment->number, "symbol dictionary specified invalid huffman table"); break; } i.e. we no longer return after the fatal error. Putting this return back in is what solves the problem. If this was done to solve a warning about the 'break;' being unreachable, would it not be better to leave the return in, and take the break out? This is the solution I will pursue now, unless you can tell me that this is wrong too. Or should it maybe have a 'goto cleanup;' in there ? Thanks!
(In reply to comment #12) > Or should it maybe have a 'goto cleanup;' in there ? "goto cleanup;" was what I was expecting to happen in the check right after the switch. It looks like the actual issue is that the local params variable is never properly initialized which may lead to freeing random memory during cleanup. Does Jbig2SymbolDictParams params = { 0 }; and then replacing the added returns with either "break" or "goto cleanup" also fix the issue for you?
OK, humble pie time. It looks like something must have gone wrong with my testing earlier. The lack of the return here is not the determining factor on whether this file crashes or not. I must have rebuilt the wrong thing or something. I will put the code back as it was. You already blank params via a memset call, so the additional init = { 0 } is not required. Sorry.