Bug 30228

Summary: poppler: use of uninitialized DCTScanInfo dcHuffTable / acHuffTable values
Product: poppler Reporter: Tomas Hoger <thoger>
Component: generalAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: minor    
Priority: low    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Reproducer

Description Tomas Hoger 2010-09-16 06:50:30 UTC
Due to the way DCTStream::readScanInfo tries to work around problems with some broken DCT streams, it can leave certain scanInfo.dcHuffTable / scanInfo.acHuffTable values uninitialized.  These values are later used as indices to DCTStream's dcHuffTables / acHuffTables arrays.  Bogus values cause out-of-bounds array access, resulting in invalid DCTHuffTable pointer being passed to DCTStream::readHuffSym, which can lead to crash.

A fix may ensure that all scanInfo.dcHuffTable / scanInfo.acHuffTable members are initialized to a safe value (e.g. 0) early in DCTStream::readScanInfo (this may not be perfect fix, but may be more corrupted-file friendly), or add an extra check near the end of DCTStream::readScanInfo verifying that all values are in range.  Upper bound can be either sizeof([ad]cHuffTables), or num[AD]CHuffTables.  Looking at the DCTStream::readHeader, there does not seem to be a guarantee that DCTStream::readHuffmanTables is called before DCTStream::readScanInfo, but it should not be called after.

Note: I do understand this is in the #ifndef ENABLE_LIBJPEG, so not too likely to be used in current poppler builds.  Feel free to wontfix if the code is considered dead.
Comment 1 Tomas Hoger 2010-09-16 06:51:55 UTC
Created attachment 38745 [details]
Reproducer

This crashes pdfimages from poppler build with --disable-libjpeg.
Comment 2 Albert Astals Cid 2010-09-16 12:32:35 UTC
If you can provide a patch i can run it through my regression test and see if it breaks something, otherwise you are right that this is not very prioritary since libjpeg works
Comment 3 Tomas Hoger 2010-09-28 12:40:56 UTC
I believe something like this should do:

diff --git a/poppler/Stream.cc b/poppler/Stream.cc
index 988f99a..399131b 100644
--- a/poppler/Stream.cc
+++ b/poppler/Stream.cc
@@ -3297,6 +3297,7 @@ GBool DCTStream::readScanInfo() {
   interleaved = scanInfo.numComps == numComps;
   for (j = 0; j < numComps; ++j) {
     scanInfo.comp[j] = gFalse;
+    scanInfo.dcHuffTable[j] = scanInfo.acHuffTable[j] = 0;
   }
   for (i = 0; i < scanInfo.numComps; ++i) {
     id = str->getChar();

This resolves the crash on the attached reproducer.
Comment 4 Albert Astals Cid 2010-09-28 12:53:32 UTC
Let's see if it doesn't break the regression test...
Comment 5 Albert Astals Cid 2010-09-28 12:55:33 UTC
BTW, may i ask if/why redhat builds poppler without libjpeg?
Comment 6 Tomas Hoger 2010-09-29 00:55:30 UTC
(In reply to comment #5)
> BTW, may i ask if/why redhat builds poppler without libjpeg?

I was looking into a crash reported for older product version, where poppler does not use libjpeg.  Current builds in Fedora and upcoming RHEL6 are configured with --enable-libjpeg and --enable-libopenjpeg.

I decided to report this issue after confirming it affects current git too, knowing that you may prefer to wontfix due to reasons already mentioned.
Comment 7 Albert Astals Cid 2010-09-29 12:05:13 UTC
I've commited the patch, thanks :-)

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.