Infinite loop in Frame::ParseTrailer

[chluo1997]

version: latest commit 14bf94c
poc: poc.zip
command: ./jpeg poc /dev/null

The backtrace in gdb:

(gdb) bt
#0  ByteStream::Get (this=0x790ae0) at bytestream.cpp:223
#1  0x000000000042331e in IOStream::PeekWord (this=0x790ae0)
    at iostream.cpp:543
#2  0x00000000004c38d5 in Frame::ParseTrailer (this=0x792590, io=0x790ae0)
    at frame.cpp:1018
#3  0x000000000043aac9 in JPEG::ReadInternal (this=0x7904c8,
    tags=0x7fffffffdd50) at jpeg.cpp:332
#4  0x000000000043988b in JPEG::Read (this=0x7904c8, tags=0x7fffffffdd50)
    at jpeg.cpp:210
#5  0x000000000041cabb in Reconstruct (infile=<optimized out>,
    outfile=0x7fffffffe70c "/dev/null", colortrafo=1, alpha=0x0, upsample=true)
    at reconstruct.cpp:121
#6  0x0000000000408b6a in main (argc=<optimized out>, argv=0x790f29)
    at main.cpp:747

Root cause:

There is a loop in frame.cpp:1017-1118.

In line 1018, the program continuously reads marker in the stream by calling IOStream::PeekWord() function.

libjpeg/marker/frame.cpp

Line 1018 in 842c7ba

LONG marker = io->PeekWord();

In cases of the default branch, the loop won't exit.

libjpeg/marker/frame.cpp

Lines 1088 to 1117 in 842c7ba

	if (marker < 0xff00) {
	JPG_WARN(MALFORMED_STREAM,"Frame::ParseTrailer",
	"expecting a marker or marker segment - stream is out of sync");
	// Advance to the next marker and see how it goes from there...
	io->Get(); // Remove the invalid thing.
	do {
	marker = io->Get();
	} while(marker != 0xff && marker != ByteStream::EOF);
	//
	if (marker == ByteStream::EOF) {
	JPG_WARN(UNEXPECTED_EOF,"Frame::ParseTrailer",
	"run into an EOF while scanning for the next marker");
	return false;
	}
	io->LastUnDo();
	// Continue parsing, check what the next marker might be.
	} else {
	// Something that looks like a valid marker. This could be the
	// tables/misc section of the next frame or next scan, depending
	// on whether we are hierarchical (next frame) or progressive (next scan).
	// Unfortunately, what is what we only know after having received either
	// the SOS marker (next scan) or an SOF marker (next frame).
	// Thus, at this time, parse of the tables, place its data in the
	// global table namespace, overriding what was there, then
	// continue parsing here until we know what we have.
	assert(m_pTables);
	// This might include EXP if we are hierarchical.
	m_pTables->ParseTables(io,NULL,m_pParent->isHierarchical(),(m_Type == JPEG_LS)?true:false);
	}
	}

The problem is that the IOStream::PeekWord() function might always return a same value.

Specifically, the IOStream::PeekWord() calls ByteStream::Get().

libjpeg/io/bytestream.cpp

Lines 214 to 224 in 91985dc

	LONG ByteStream::Get(void) // read a single byte (not inlined)
	{
	if (m_pucBufPtr >= m_pucBufEnd) {
	if (Fill() == 0) // Found EOF
	return EOF;
	}
	assert(m_pucBufPtr < m_pucBufEnd);
	return *m_pucBufPtr++;
	}

IOStream::PeekWord() returns at line 223 with m_pucBufPtr++. However, when using gdb to check the value of m_pucBufPtr, I found that the ByteStream::Get() functions repeatedly read the values from the same address. The 0x790f2a and 0x790f29 correspond to byte1 and byte2 in IOStream::PeekWord() and they never change.

IOStream::PeekWord() then always returns a same value (calculated by byte1 and byte2). Finally, the program never terminates because the return value forces the program to take the default branch.

[thorfdbg]

Thank you, this should be fixed in the latest trunk.