Potential - small - race condition in gdalrasterband.cpp ?

[Even Rouault]

Frank,

While reviewing #3224, I ended up reviewing the current implementation in gdalrasterband.cpp and (I think) I've discovered a (probably very hard-to-hit) race condition. The scenario is Thread 1 flushing a block (from FlushCacheBlock() for example) while Thread 2 calling TryGetLockedBlockRef() on the same block.

Below a potential executation flow that could lead to crashes :

Thread 1                                                                Thread 2

FlushBlock()                                                            TryGetLockedBlockRef()

1000:  GDALRasterBlock::SafeLockBlock( papoBlocks + nBlockIndex );
                                                                        1117: GDALRasterBlock::SafeLockBlock( papoBlocks + nBlockIndex );
                                                                        1119: return papoBlocks[nBlockIndex];
1002:  poBlock = papoBlocks[nBlockIndex];                               
1003:  papoBlocks[nBlockIndex] = NULL;
.....
1049:  delete poBlock;
                                                                        XXXX : uses non value returned by line 1119, which is now destroyed

The scope of the mutex in SafeLockBlock() is not sufficient. The same applies for the sub-blocking case, so there should be a mutex protecting lines 1000-1003, 1014-1029, 1117-1119 and 1125-1142

[Even Rouault]

Yes, I've confirmed that there's an issue with the attached version of gdalrasterband.cpp that is patched to add sleeps at strategic places.

Here's the output of a run of hit_bug_3225.c :

$  ./hit_bug_3225.exe
cache fill
waiting at line 1211
end of waiting at line 1213
using 00000000
end of cache fill
buf[0]=107

begin thread
waiting at line 1010
re read block
waiting at line 1211
end of waiting at line 1012
deleting 01076F88
ERROR 7: Assertion `nLockCount == 0' failed
in file `gdalrasterblock.cpp', line 282

[Even Rouault]

Hacked version that adds 2 sleeps

[Even Rouault]

I believe this hole has been closed by the work done in GDAL 2.0. GDALRasterBand::FlushBlock() is no longer called from GDALRasterBlock. Instead the new GDALRasterBand::UnreferenceBlock() is called by GDALRasterBlock and always with the RB mutex taken.

[Even Rouault]

Milestone renamed