[PATCH] PNG driver abort()s on some corrupt png files.

[dmorissette]

Running gdal_translate under valgrind:

valgrind --leak-check=yes gdal_translate -of PNG test.xml /tmp/test.png

... on the following WMS input file which contains an invalid server URL:

<GDAL_WMS>
    <Service name="WMS">
        <Version>1.1.1</Version>
        <ServerUrl>http://onearth.jpl.nasa.gov/wms.cgi_invalid?</ServerUrl>
        <SRS>EPSG:4326</SRS>
        <ImageFormat>image/jpeg</ImageFormat>
        <Layers>modis,global_mosaic</Layers>
        <Styles></Styles>
    </Service>
    <DataWindow>
        <UpperLeftX>-180.0</UpperLeftX>
        <UpperLeftY>90.0</UpperLeftY>
        <LowerRightX>180.0</LowerRightX>
        <LowerRightY>-90.0</LowerRightY>
        <SizeX>1000</SizeX>
        <SizeY>1000</SizeY>
    </DataWindow>
    <Projection>EPSG:4326</Projection>
    <BandsCount>3</BandsCount>
</GDAL_WMS>

... results in the following errors (of course):

ERROR 1: IReadBlock failed at X offset 0, Y offset 0
ERROR 1: GetBlockRef failed at X block offset 0, Y block offset 0
ERROR 1: IReadBlock failed at X offset 0, Y offset 0
ERROR 1: GetBlockRef failed at X block offset 0, Y block offset 0
ERROR 1: IReadBlock failed at X offset 0, Y offset 0
ERROR 1: GetBlockRef failed at X block offset 0, Y block offset 0
libpng error: No IDATs written into file

... and in the following leaks being reported by valgrind:

==10800== 1,759 bytes in 81 blocks are possibly lost in loss record 89 of 97
==10800==    at 0x401C7F3: operator new(unsigned) (vg_replace_malloc.c:164)
==10800==    by 0x4DDD489: std::string::_Rep::_S_create(unsigned, unsigned, std::allocator<char> const&) (in /media/hda5/data-ubuntu/opt/fgs1/fgs/lib/libstdc++.so.6.0.7)
==10800==    by 0x4DDD94F: (within /media/hda5/data-ubuntu/opt/fgs1/fgs/lib/libstdc++.so.6.0.7)
==10800==    by 0x4DDDAFC: std::string::string(char const*, std::allocator<char> const&) (in /media/hda5/data-ubuntu/opt/fgs1/fgs/lib/libstdc++.so.6.0.7)
==10800==    by 0x42B7798: VSIInstallMemFileHandler (cpl_vsi_mem.cpp:616)
==10800==    by 0x42B4852: VSIFileManager::Get() (cpl_vsil.cpp:574)
==10800==    by 0x42B4877: VSIFileManager::GetHandler(char const*) (cpl_vsil.cpp:587)
==10800==    by 0x42B4B40: VSIReadDir (cpl_vsil.cpp:62)
==10800==    by 0x427D514: GDALDriverManager::AutoLoadDrivers() (gdaldrivermanager.cpp:595)
==10800==    by 0x416977E: GDALAllRegister (gdalallregister.cpp:75)
==10800==    by 0x8049A4A: ProxyMain(int, char**) (gdal_translate.cpp:129)
==10800==    by 0x804BEFF: main (gdal_translate.cpp:865)

It would be good to trap the errors and report a more meaningful error message to the user, but more importantly the leak needs to be tracked and fixed.

[warmerdam]

This seems to be an issue with the png driver recovering from fatal errors. Currently libpng is just calling abort() or something.

warmerda@amd64[3]% gdal_translate -of PNG test.wms out.png
GDAL: GDALOpen(test.wms) succeeds as WMS.
Input file size is 1000, 1000
ERROR 1: IReadBlock failed at X offset 0, Y offset 0
ERROR 1: GetBlockRef failed at X block offset 0, Y block offset 0
ERROR 1: IReadBlock failed at X offset 0, Y offset 0
ERROR 1: GetBlockRef failed at X block offset 0, Y block offset 0
ERROR 1: IReadBlock failed at X offset 0, Y offset 0
ERROR 1: GetBlockRef failed at X block offset 0, Y block offset 0
libpng error: No IDATs written into file
Abort (core dumped)

Mateusz, could you look into ways of catching this sort of error condition? We already do a bunch of stuff installing png_gdal_error() to trap fatal errors, so I don't know why this isn't caught. We should consider retrofitting a fix to 1.4.x if one is found.

[rouault]

I'm attaching a patch that should fix the issue. The reason why this wasn't caught is that the png_gdal_error trap was only installed for opening PNG datasets and not in PNGCreateCopy. I had to rework a bit png_gdal_error since in PNGCreateCopy we have no valid dataset object and thus cannot access the sSetJmpBuf member.

I'd like also to draw attention on the fact that error trap is probably not correct when SUPPORT_CREATE is enabled in PNGDataset::write_png_header. png_create_write_struct is called with a NULL user pointer. And I don't think we can call setjmp in write_png_header since errors may arise after this method has returned. But that's another story...

[warmerdam]

Even / Mateusz,

The patch looks reasonable. What's with the GdalPNGErrorJmpBufWrappingStruct? This part of the change seems ... gratuitous.

I've added RayG to the cc: list so he can contemplate SUPPORT_CREATE issues which are not used in normal GDAL builds.

[rouault]

Daniel,

I didn't like my GdalPNGErrorJmpBufWrappingStruct stuff very much too but I was struggling yesterday with dereferencing a pointer of type jmp_buf* in C++ (I think it's because jmp_buf is in fact an array[1] of something.) Today brought to me a better inspiration and I've updated the patch with something more good-looking hopefully...

[dmorissette]

I have verified that the latest patch fixes the leak that I had reported. However, I'll leave it up to Mateusz and Frank to review/comment on the patch and eventually apply it in SVN.

[warmerdam]

Mateusz,

I think Even's patch looks fine and can be applied in trunk. I gather the problem occurs with partial (truncated) png files. Please add a corrupt png file and corresponding test to the gdalautotest suite that will verify this error trapping machinery is working.

[mloskot]

Patch applied to trunk (r13366)

[mloskot]

I updated png.py tests (r13371)

[mloskot]

Backported to branches/1.5 (r13372)