Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#2523 closed defect (fixed)

GDALRasterBand::ReadBlock(bx, by, ...) fails if the block has not been written

Reported by: stmoebius Owned by: Even Rouault
Priority: normal Milestone: 1.5.3
Component: GDAL_Raster Version: 1.5.1
Severity: normal Keywords: hfa empty compress
Cc: Mateusz Łoskot

Description

GDALRasterBand::ReadBlock(bx, by, ...) fails if GDALRasterBand::WriteBlock(bx, by, ...) was never called for block bx, by

Cause: GDALRasterBand::ReadBlock() correctly determines that the block size is 0 but then tries to read 1 block of size 0 from the file.

Expected behavior: GDALRasterBand::ReadBlock() should initialize the block with no-data values.

Change History (8)

comment:1 Changed 11 years ago by warmerdam

Stefan,

I'm going to need more details to look into this. What driver are you using, for instance. What sequence of steps is needed to reproduce the problem?

Generally speaking after a create the read block on a previously unwritten block is expected to return valid image data that is all zeros, not a special nodata value.

comment:2 Changed 11 years ago by stmoebius

[Apologies. I was just passing on the request from an experienced developer (or so I thought)]

As far as I know we are talking about the HFA driver. And what we do is to put a number of small images into one large image, such that the small ones may not fill up the large image. Later on, we try to read from the large image, "failing" in those areas that haven't been written to in the first step.

I'll return with more information (but only next week), such as: (1) In what way does it "fail"? (2) Has the file been closed between writing and reading?

Any other information that would be useful?

comment:3 Changed 11 years ago by stmoebius

Alright, some more information:

Yes, the file has been closed before reading.

The problem seems to occur in HFABand::GetRasterBlock(), specifically around line 832:

/* -------------------------------------------------------------------- */
/*      If the block isn't valid, we just return all zeros, and an	*/
/*	indication of success.                        			*/
/* -------------------------------------------------------------------- */
    if( !panBlockFlag[iBlock] & BFLG_VALID )
    {
        memset( pData, 0, 
                HFAGetDataTypeBits(nDataType)*nBlockXSize*nBlockYSize/8 );

        return( CE_None );
    }

The if statement looks bogus and in our case causes execution to proceed, and later on nBlockSize becomes 0 causing a CPLError.

Please let me know if you need further details.

comment:4 Changed 11 years ago by Even Rouault

Owner: changed from warmerdam to Even Rouault

comment:5 Changed 11 years ago by Even Rouault

Keywords: hfa empty compress added
Milestone: 1.5.3
Resolution: fixed
Status: newclosed

Yes the test was buggy. The problem was only hit with a compressed and empty file.

Fixed in trunk in r15163, in branches/1.5 in r15164. Test added in r15165

comment:6 Changed 11 years ago by Mateusz Łoskot

Cc: Mateusz Łoskot added

Similar bug exist in ogrdodssequencelayer.cpp file, near lines 995 and 1017. There are conditions like this:

if( pabyValToCheck[3] & 0x7f == 0x7f 
    && pabyValToCheck[2] & 0x80 == 0x80 )

Operator == has higher precedence than &, so the above evaluates to

pabyValToCheck[3] & 1 

and

pabyValToCheck[2] & 1

and the whole expression is always true for odd values of 2nd and 3rd array elements.

Parentheses are suggested.

comment:7 Changed 11 years ago by Even Rouault

Thanks for noticing Mateusz.

Fixed in trunk in r15172 and in branches/1.5 in r15173

comment:8 Changed 11 years ago by Mateusz Łoskot

Even,

Thanks for fixing.

FYI, I've scanned and reviewed GDAL sources regarding such kind of suspicious constructions this way:

$ export GREP_COLOR="1;32"
find trunk/gdal -name '*.c' | xargs grep --colour -n ' & .*=='
find trunk/gdal -name '*.cpp' | xargs grep --colour -n ' & .*=='
Note: See TracTickets for help on using tickets.