Bug 30228

Summary:	poppler: use of uninitialized DCTScanInfo dcHuffTable / acHuffTable values
Product:	poppler	Reporter:	Tomas Hoger <thoger>
Component:	general	Assignee:	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.