Ticket #1769 (closed defect: fixed)

Opened 9 months ago

Last modified 5 months ago

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

Reported by: dmorissette Assigned to: mloskot
Priority: high Milestone: 1.5.0
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: png
Cc: warmerdam, nowakpl, rayg, 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

gdal_svn_trunk_1769.patch (2.1 kB) - added by rouault on 09/10/07 14:02:34.

Change History

08/28/07 11:14:22 changed by warmerdam

  • keywords set to png.
  • owner changed from nowakpl to mloskot.
  • version changed from unspecified to svn-trunk.
  • component changed from default to GDAL_Raster.
  • cc changed from warmerdam to warmerdam, nowakpl.

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.

09/09/07 14:03:19 changed by 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...

09/10/07 11:05:36 changed by warmerdam

  • cc changed from warmerdam, nowakpl to warmerdam, nowakpl, rayg.

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.

09/10/07 14:01:54 changed by 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...

09/10/07 14:02:34 changed by rouault

  • attachment gdal_svn_trunk_1769.patch added.

09/10/07 14:53:49 changed by 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.

10/12/07 11:26:48 changed by rouault

  • summary changed from WMS driver leak when GetMap requests return unreadable results to [PATCH] WMS driver leak when GetMap requests return unreadable results.

10/12/07 13:12:00 changed by warmerdam

  • cc changed from warmerdam, nowakpl, rayg to warmerdam, nowakpl, rayg, rouault.

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.

10/12/07 13:13:07 changed by warmerdam

  • summary changed from [PATCH] WMS driver leak when GetMap requests return unreadable results to [PATCH] PNG driver abort()s on some corrupt png files..

11/15/07 11:16:32 changed by mloskot

  • status changed from new to assigned.

12/11/07 09:31:55 changed by warmerdam

  • priority changed from normal to high.

12/18/07 16:35:18 changed by mloskot

  • status changed from assigned to closed.
  • resolution set to fixed.

Patch applied to trunk (r13366)

12/18/07 17:17:27 changed by mloskot

I updated png.py tests (r13371)

12/18/07 17:35:52 changed by mloskot

Backported to branches/1.5 (r13372)