[BUG] pngfix infinite loop

[SophrosyneX]

Crash Inputs

Here is the file that trigger the error pngfix_dead_loop.zip reproduce in Ubuntu 18.04, latest libpng repo

Bug Description:

When executing the pngfix program with the given input file, the program does not finish after waiting for several hours, it keeps running, but there is no additional output either. The program output:

IDAT OK  maximum 9 9 148 288 /path/to/input

Step to reproduce:

• clone the latest libpng: git clone https://github.com/glennrp/libpng.git
• compile the libpng
• execute pngfix with target program: pngfix ./input
• maybe wait for several hours...

[thealberto]

Hi @SophrosyneX , Thanks for reporting it. I'll try to understand why there is an infinite loop and try to fix it.

Thanks

[SophrosyneX]

Hi @SophrosyneX , Thanks for reporting it. I'll try to understand why there is an infinite loop and try to fix it.

Thanks

Thank you!

[SophrosyneX]

Hi @thealberto, after debugging we find that every time the program jumps into function png_read_data (in pngrio.c), buf is always "\0\0\0\0IDAT". As a result, the value of png_get_uint_31 is 0, which leads to the infinite loop of program pngfix. Following pictures are bug location and debugging results.

Fig1. infinite loop location

Fig2. the value of png_get_uint_31 is always 0

Fig3. funct png_read_data in pngrio.c

[thealberto]

Hi @SophrosyneX , thanks for the update. This is an interesting bug for sure. I'm not sure how to fix but maybe together we can find a way :)

Just to be clear, I would like to help but I'm not an official contributor atm.

In my opinion an IDAT with length 0 should be causing an error since there is no data...

@ctruta

[ctruta]

Hello, and thank you for the report.

The error is in pngfix.c, not in libpng. By default, libpng skips all IDAT chunks of size 0, and then it raises a proper png_error when those IDAT chunks run out.

Even with a custom file reader that works correctly, libpng works correctly also.

However, in pngfix.c, file reading is a highly-convoluted function that pretends to read a file stream but in fact it does a lot more things. See, for example, the function read_callback(). This is where caching of IDAT becomes confusing, and where that function ends up returning an infinite supply of IDAT chunks, all of them of size zero, and all of them for libpng to process without knowing that the supply of IDATs is in fact infinite.

[thealberto]

Hi @ctruta , are you interested to have a fix for it? Any pointer on how to approach it?

Thanks

[jbowler]

It's a bug; zero length IDATs are valid and actually have a certain use in real PNG files to arrange for the IDAT chunks to be on a mmap'ed page boundary. That said I can't see why pngfix needs to preserve them; it can already change the IDAT size IRC. In this case the IDAT is at the end of a truncated file and the loop apparently results from the corresponding look over zero length IDATs in pngrutil.c

So I think pngfix needs to not deliver zero length IDAT chunks to pngrutil.c even if it encounters them but even if it does it clearly should not keep doing so when it gets to the end of the input...

I've got it in the debugger, still trying to track down where the logic error is, the file is truncated so it shouldn't need to do a resync because there is no zlib data there to resync to.