Repair of CVE-2018-14048 #238

[tangyaofang]

Repair of CVE-2018-14048 #238

[heaven-hq]

@ctruta I have verified this PR. When an abnormal situation is entered, it will enter

if (setjmp (png_jmpbuf(png_ptr)))
{
    png_destroy_read_struct (&png_ptr, &info_ptr, NULL);  
    return FALSE;
}

branch and cause memory leak.This patch could solve this problem very well. Do you plan to merge this PR?

[vesalvojdani]

Note: This has nothing to do with the SEGV "vulnerability", but it does fix the ASAN reports of a memory leak within png2pnm, which people keep reporting. The fix requires just two lines of freeing row_pointers and png_pixels. Also compare:

https://github.com/glennrp/libpng/blob/f8e5fa92b0e37ab597616f554bee254157998227/contrib/pngminus/pnm2png.c#L463-L468

This is not a serious leak, but many issues are kept open because of this:

• It was a confounding factor in #238
• Separate issue in #239
• Further complaint that it's not fixed #288

[jbowler]

@wwwillem you may care to comment.

The program immediately exits after the setjmp returns non-zero, but then why call png_destroy_read_struct?

It's valid in ANSI-C to do so because png_ptr is not modified after the initial call to setjmp (the one that returns 0), but it's misleading as documentation because other autos that are modified after the initial call to setjmp are not freed (and inserting calls to free them will result in warnings on some compilers).

The code as written is completely correct; the program exits after the error and that frees all the memory.

This code is just documentation; someone who copies it and doesn't understand all the issues that surround setjmp is worthy of a CVE but that is still nothing to do with libpng.

NOT a libpng issue @ctruta

[ctruta]

I confirm that CVE-2018-14048 is invalid, although I can understand how it is far from obvious that libpng is not leaky. After all, the png and info structures are being cleaned up in both png2pnm.c and pnm2png.c at the end of processing; and yet, there's an ASan report -- and I can understand that it looks suspicious.

In any case (FYI @wwwillem) I just pushed a bunch of rather significant changes to the pngminus suite. See the commits c993ae4c67e99dbed65258412794be5e071b8f95, abb8d4a71fb58e9988d38dd9d726c2e947d0893f, bdbbcaa4578f804734ddc6678fb24378cc18ebcf.

In conclusion:

Neither CVE-2018-14048 nor the memory leak reported by ASan should be problematic any further.