Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#1769 closed defect (fixed)

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

Reported by: Daniel Morissette Owned by: Mateusz Łoskot
Priority: high Milestone: 1.5.0
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: png
Cc: warmerdam, nowakpl, rayg, Even Rouault

Description

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.

Attachments (1)

gdal_svn_trunk_1769.patch (2.1 KB) - added by Even Rouault 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by warmerdam

Cc: nowakpl added
Component: defaultGDAL_Raster
Keywords: png added
Owner: changed from nowakpl to Mateusz Łoskot
Version: unspecifiedsvn-trunk

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.

comment:2 Changed 11 years ago by Even 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...

comment:3 Changed 11 years ago by warmerdam

Cc: rayg added

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.

comment:4 Changed 11 years ago by Even 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...

Changed 11 years ago by Even Rouault

Attachment: gdal_svn_trunk_1769.patch added

comment:5 Changed 11 years ago by Daniel Morissette

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.

comment:6 Changed 11 years ago by Even Rouault

Summary: WMS driver leak when GetMap requests return unreadable results[PATCH] WMS driver leak when GetMap requests return unreadable results

comment:7 Changed 11 years ago by warmerdam

Cc: Even Rouault added

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.

comment:8 Changed 11 years ago by warmerdam

Summary: [PATCH] WMS driver leak when GetMap requests return unreadable results[PATCH] PNG driver abort()s on some corrupt png files.

comment:9 Changed 11 years ago by Mateusz Łoskot

Status: newassigned

comment:10 Changed 11 years ago by warmerdam

Priority: normalhigh

comment:11 Changed 11 years ago by Mateusz Łoskot

Resolution: fixed
Status: assignedclosed

Patch applied to trunk (r13366)

comment:12 Changed 11 years ago by Mateusz Łoskot

I updated png.py tests (r13371)

comment:13 Changed 11 years ago by Mateusz Łoskot

Backported to branches/1.5 (r13372)

Note: See TracTickets for help on using tickets.