Opened 15 years ago

Closed 15 years ago

#2807 closed defect (fixed)

[PATCH] Fix parameter validation and memory allocation in INGR driver

Reported by: Even Rouault Owned by: ilucena
Priority: normal Milestone: 1.7.0
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords: ingr
Cc:

Description

Ivan,

I'm attaching a patch that adds some validation of values read from INGR files that have influence on memory allocations, and prevent from a few crashes in presence of corrupted/hostile files. Most changes are purely mechanical : use VSIMalloc/Calloc instead of CPLMalloc/Calloc as the first one is non fatal, and check the returned pointer against NULL.

However, I'd like that you confirm that my changes, in particularly the one related to missing tile blocks in IntergraphRasterBand::LoadBlockBuf() and its calling functions is correct. I'm not sure that this code path is tested by one of the tests in autotest/gdrivers/ingr.py

The current code which is :

    // --------------------------------------------------------------------
    // Read from tiles or read from strip
    // --------------------------------------------------------------------

    if( bTiled )
    {
        nBlockId = nBlockXOff + nBlockYOff * nBlocksPerRow;

         if( pahTiles[nBlockId].Start == 0 ) 
         {
            // ------------------------------------------------------------
            // Uninstantieted tile, unique value
            // ------------------------------------------------------------

            memset( pabyBlock, pahTiles[nBlockId].Used, nBlockBufSize );
            return nBlockBufSize;
         }

seems wrong in the memset. pahTiles[nBlockId].Used seems to be a number of bytes, and not an init value. And there's no guarantee that the pabyBlock buffer is at least nBlockBufSize. I could manage to trigger a Valgrind error on a RLE dataset, because in that case pabyBlock == pabyRLEBlock, which is nRLESize, and possibly I think < nBlockBufSize. My change is to move and change that test to the calling functions where they do :

        if( bTiled && pahTiles[nBlockXOff + nBlockYOff * nBlocksPerRow].Start == 0 ) 
        {
            memset( pImage, 0, nBlockBufSize );
            return CE_None;
        }

Attachments (2)

ITG.zip (236.8 KB ) - added by ilucena 15 years ago.
Intergraph Raster Format Reference Guide Mar/1994
ingr_ticket2807.patch (21.6 KB ) - added by Even Rouault 15 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by ilucena, 15 years ago

Hi there,

It will be difficult to find data sample that used uninstantiated but I can try to find the documentation that backs up the use of "Used" as a value in that special case. If I remember correctly it says that if a tile is completely filled by the same pixel value you can save space by storing just that notation on the tile directory (Start = 0 and Used is the value). But at that point GDAL is requesting a full block so that is what memset() and "return nBlockBufSize" is doing.

comment:2 by Even Rouault, 15 years ago

Ivan,

hum, ok. I trust you that Used can be used as a value in that special case. However the nBlockBufSize as the third argument of the memset in the current code is wrong. I could cause a crash by just changing the value at offset 10880 of frmt09t.cot from 0xb0 to 0x00.

==23965== Invalid write of size 1
==23965==    at 0x4023D6A: memset (mc_replace_strmem.c:492)
==23965==    by 0x4147E58: IntergraphRasterBand::LoadBlockBuf(int, int, int, unsigned char*) (IntergraphBand.cpp:844)
==23965==    by 0x4148454: IntergraphRLEBand::IReadBlock(int, int, void*) (IntergraphBand.cpp:540)
==23965==    by 0x4363AFF: GDALRasterBand::GetLockedBlockRef(int, int, int) (gdalrasterband.cpp:1125)
==23965==    by 0x436CCFB: GDALRasterBand::IRasterIO(GDALRWFlag, int, int, int, int, void*, int, int, GDALDataType, int, int) (rasterio.cpp:217)
==23965==    by 0x4362EAD: GDALRasterBand::RasterIO(GDALRWFlag, int, int, int, int, void*, int, int, GDALDataType, int, int) (gdalrasterband.cpp:241)
==23965==    by 0x4362F6A: GDALRasterIO (gdalrasterband.cpp:266)
==23965==    by 0x439471C: GDALChecksumImage (gdalchecksum.cpp:147)
==23965==    by 0x804AF89: main (gdalinfo.c:477)

Perhaps, I should just modified my patch to replace the 0 used as the init value of memset by pahTiles[nBlockId].Used ?

by ilucena, 15 years ago

Attachment: ITG.zip added

Intergraph Raster Format Reference Guide Mar/1994

by Even Rouault, 15 years ago

Attachment: ingr_ticket2807.patch added

comment:3 by Even Rouault, 15 years ago

Ivan, thanks for the documentation. I've updated my patch accordingly for uninstanciated tiles.

I've applied it in trunk in r16211. The autotest suite still passes fine. Maybe you have other Intergraph data to check that I haven't break anything.

comment:4 by ilucena, 15 years ago

Even,

That document is not available at ftp.intergraph.com anymore but I found it on my old computer.

I am thinking about doing a revision on the JPEG compression. They use a non-official abbreviated JPEG format (without any JPEG table at all) and I basically needed to use reverse engineer and lots of hours on the hexadecimal editor to get it to work but it is still imperfect.

I am going to run some tests with large data that usually doesn't fit on gdalautotest.

Thank you for doing those improvements.

comment:5 by Even Rouault, 15 years ago

Additionnal fix in r16391 as the memset was incorrect in some cases because nBlockBufSize could be larger than the band buffer size.

comment:6 by Even Rouault, 15 years ago

Milestone: 1.7.0
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.