Skip to content

Commit

Permalink
If an error occurs while reading raster block data, don't return
Browse files Browse the repository at this point in the history 
corrupt data as a result. Instead, indicate explicitly that an
error occurred so that callers will fallback on appropriate
error paths.

Fixes rendering random junk (and possible crashes) when
attempting to open an invalid gdal raster data source,
such as the one attached to
OSGeo/gdal#1545

Also replace manual memory management with unique_ptrs
  • Loading branch information
@nyalldawson
nyalldawson committed May 21, 2019
1 parent e5ab06f commit 230c62f
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 54 deletions.
31 changes: 19 additions & 12 deletions src/core/raster/qgsrasterdataprovider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
QgsDebugMsgLevel( QStringLiteral( "bandNo = %1 width = %2 height = %3" ).arg( bandNo ).arg( width ).arg( height ), 4 );
QgsDebugMsgLevel( QStringLiteral( "boundingBox = %1" ).arg( boundingBox.toString() ), 4 );

QgsRasterBlock *block = new QgsRasterBlock( dataType( bandNo ), width, height );
std::unique_ptr< QgsRasterBlock > block = qgis::make_unique< QgsRasterBlock >( dataType( bandNo ), width, height );
if ( sourceHasNoDataValue( bandNo ) && useSourceNoDataValue( bandNo ) )
{
block->setNoDataValue( sourceNoDataValue( bandNo ) );
Expand All @@ -55,7 +55,7 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
if ( block->isEmpty() )
{
QgsDebugMsg( QStringLiteral( "Couldn't create raster block" ) );
return block;
return block.release();
}

// Read necessary extent only
Expand All @@ -65,7 +65,7 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
{
QgsDebugMsg( QStringLiteral( "Extent outside provider extent" ) );
block->setIsNoData();
return block;
return block.release();
}

double xRes = boundingBox.width() / width;
Expand Down Expand Up @@ -112,7 +112,7 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
{
// Should not happen
QgsDebugMsg( QStringLiteral( "Row or column limits out of range" ) );
return block;
return block.release();
}

// If lower source resolution is used, the extent must beS aligned to original
Expand All @@ -139,13 +139,18 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
QgsDebugMsgLevel( QStringLiteral( "Reading smaller block tmpWidth = %1 height = %2" ).arg( tmpWidth ).arg( tmpHeight ), 4 );
QgsDebugMsgLevel( QStringLiteral( "tmpExtent = %1" ).arg( tmpExtent.toString() ), 4 );

QgsRasterBlock *tmpBlock = new QgsRasterBlock( dataType( bandNo ), tmpWidth, tmpHeight );
std::unique_ptr< QgsRasterBlock > tmpBlock = qgis::make_unique< QgsRasterBlock >( dataType( bandNo ), tmpWidth, tmpHeight );
if ( sourceHasNoDataValue( bandNo ) && useSourceNoDataValue( bandNo ) )
{
tmpBlock->setNoDataValue( sourceNoDataValue( bandNo ) );
}

readBlock( bandNo, tmpExtent, tmpWidth, tmpHeight, tmpBlock->bits(), feedback );
if ( !readBlock( bandNo, tmpExtent, tmpWidth, tmpHeight, tmpBlock->bits(), feedback ) )
{
QgsDebugMsg( QStringLiteral( "Error occurred while reading block" ) );
block->setIsNoData();
return block.release();
}

int pixelSize = dataTypeSize( bandNo );

Expand All @@ -168,8 +173,7 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
{
QgsDebugMsg( QStringLiteral( "Source row or column limits out of range" ) );
block->setIsNoData(); // so that the problem becomes obvious and fixed
delete tmpBlock;
return block;
return block.release();
}

qgssize tmpIndex = static_cast< qgssize >( tmpRow ) * static_cast< qgssize >( tmpWidth ) + tmpCol;
Expand All @@ -190,19 +194,22 @@ QgsRasterBlock *QgsRasterDataProvider::block( int bandNo, QgsRectangle const &b
memcpy( bits, tmpBits, pixelSize );
}
}

delete tmpBlock;
}
else
{
readBlock( bandNo, boundingBox, width, height, block->bits(), feedback );
if ( !readBlock( bandNo, boundingBox, width, height, block->bits(), feedback ) )
{
QgsDebugMsg( QStringLiteral( "Error occurred while reading block" ) );
block->setIsNoData();
return block.release();
}
}

// apply scale and offset
block->applyScaleOffset( bandScale( bandNo ), bandOffset( bandNo ) );
// apply user no data values
block->applyNoDataValues( userNoDataValues( bandNo ) );
return block;
return block.release();
}

QgsRasterDataProvider::QgsRasterDataProvider()
Expand Down
14 changes: 8 additions & 6 deletions src/core/raster/qgsrasterdataprovider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -538,18 +538,20 @@ class CORE_EXPORT QgsRasterDataProvider : public QgsDataProvider, public QgsRast
protected:

/**
* Read block of data
* Reads a block of raster data into \a data.
* \returns TRUE if the block was successfully read, or FALSE if an error occurred and the block could not be read.
* \note not available in Python bindings
*/
virtual void readBlock( int bandNo, int xBlock, int yBlock, void *data ) SIP_SKIP
{ Q_UNUSED( bandNo ) Q_UNUSED( xBlock ); Q_UNUSED( yBlock ); Q_UNUSED( data ); }
virtual bool readBlock( int bandNo, int xBlock, int yBlock, void *data ) SIP_SKIP
{ Q_UNUSED( bandNo ) Q_UNUSED( xBlock ); Q_UNUSED( yBlock ); Q_UNUSED( data ); return false; }

/**
* Read block of data using give extent and size
* Reads a block of raster data into \a data, using the given extent and size.
* \returns TRUE if the block was successfully read, or FALSE if an error occurred and the block could not be read.
* \note not available in Python bindings
*/
virtual void readBlock( int bandNo, QgsRectangle const &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) SIP_SKIP
{ Q_UNUSED( bandNo ) Q_UNUSED( viewExtent ); Q_UNUSED( width ); Q_UNUSED( height ); Q_UNUSED( data ); Q_UNUSED( feedback ); }
virtual bool readBlock( int bandNo, QgsRectangle const &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) SIP_SKIP
{ Q_UNUSED( bandNo ) Q_UNUSED( viewExtent ); Q_UNUSED( width ); Q_UNUSED( height ); Q_UNUSED( data ); Q_UNUSED( feedback ); return false; }

//! Returns TRUE if user no data contains value
bool userNoDataValuesContains( int bandNo, double value ) const;
Expand Down
5 changes: 3 additions & 2 deletions src/providers/arcgisrest/qgsamsprovider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ QgsRasterIdentifyResult QgsAmsProvider::identify( const QgsPointXY &point, QgsRa
return QgsRasterIdentifyResult( format, entries );
}

void QgsAmsProvider::readBlock( int /*bandNo*/, const QgsRectangle &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback )
bool QgsAmsProvider::readBlock( int /*bandNo*/, const QgsRectangle &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback )
{
Q_UNUSED( feedback ) // TODO: make use of the feedback object

Expand All @@ -464,9 +464,10 @@ void QgsAmsProvider::readBlock( int /*bandNo*/, const QgsRectangle &viewExtent,
if ( mCachedImage.width() != width || mCachedImage.height() != height )
{
QgsDebugMsg( QStringLiteral( "Unexpected image size for block" ) );
return;
return false;
}
std::memcpy( data, mCachedImage.constBits(), mCachedImage.bytesPerLine() * mCachedImage.height() );
return true;
}

#ifdef HAVE_GUI
Expand Down
2 changes: 1 addition & 1 deletion src/providers/arcgisrest/qgsamsprovider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class QgsAmsProvider : public QgsRasterDataProvider
QgsRasterIdentifyResult identify( const QgsPointXY &point, QgsRaster::IdentifyFormat format, const QgsRectangle &extent = QgsRectangle(), int width = 0, int height = 0, int dpi = 96 ) override;

protected:
void readBlock( int bandNo, const QgsRectangle &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) override;
bool readBlock( int bandNo, const QgsRectangle &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) override;

void draw( const QgsRectangle &viewExtent, int pixelWidth, int pixelHeight );

Expand Down
41 changes: 27 additions & 14 deletions src/providers/gdal/qgsgdalprovider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,43 +644,48 @@ QString QgsGdalProvider::htmlMetadata()

QgsRasterBlock *QgsGdalProvider::block( int bandNo, const QgsRectangle &extent, int width, int height, QgsRasterBlockFeedback *feedback )
{
QgsRasterBlock *block = new QgsRasterBlock( dataType( bandNo ), width, height );
std::unique_ptr< QgsRasterBlock > block = qgis::make_unique< QgsRasterBlock >( dataType( bandNo ), width, height );
if ( !initIfNeeded() )
return block;
return block.release();
if ( sourceHasNoDataValue( bandNo ) && useSourceNoDataValue( bandNo ) )
{
block->setNoDataValue( sourceNoDataValue( bandNo ) );
}

if ( block->isEmpty() )
{
return block;
return block.release();
}

if ( !mExtent.intersects( extent ) )
{
// the requested extent is completely outside of the raster's extent - nothing to do
block->setIsNoData();
return block;
return block.release();
}

if ( !mExtent.contains( extent ) )
{
QRect subRect = QgsRasterBlock::subRect( extent, width, height, mExtent );
block->setIsNoDataExcept( subRect );
}
readBlock( bandNo, extent, width, height, block->bits(), feedback );
if ( !readBlock( bandNo, extent, width, height, block->bits(), feedback ) )
{
QgsDebugMsg( QStringLiteral( "Error occurred while reading block" ) );
block->setIsNoData();
return block.release();
}
// apply scale and offset
block->applyScaleOffset( bandScale( bandNo ), bandOffset( bandNo ) );
block->applyNoDataValues( userNoDataValues( bandNo ) );
return block;
return block.release();
}

void QgsGdalProvider::readBlock( int bandNo, int xBlock, int yBlock, void *data )
bool QgsGdalProvider::readBlock( int bandNo, int xBlock, int yBlock, void *data )
{
QMutexLocker locker( mpMutex );
if ( !initIfNeeded() )
return;
return false;

// TODO!!!: Check data alignment!!! May it happen that nearest value which
// is not nearest is assigned to an output cell???
Expand All @@ -694,14 +699,21 @@ void QgsGdalProvider::readBlock( int bandNo, int xBlock, int yBlock, void *data
// We have to read with correct data type consistent with other readBlock functions
int xOff = xBlock * mXBlockSize;
int yOff = yBlock * mYBlockSize;
gdalRasterIO( myGdalBand, GF_Read, xOff, yOff, mXBlockSize, mYBlockSize, data, mXBlockSize, mYBlockSize, ( GDALDataType ) mGdalDataType.at( bandNo - 1 ), 0, 0 );
CPLErr err = gdalRasterIO( myGdalBand, GF_Read, xOff, yOff, mXBlockSize, mYBlockSize, data, mXBlockSize, mYBlockSize, ( GDALDataType ) mGdalDataType.at( bandNo - 1 ), 0, 0 );
if ( err != CPLE_None )
{
QgsLogger::warning( "RasterIO error: " + QString::fromUtf8( CPLGetLastErrorMsg() ) );
return false;
}

return true;
}

void QgsGdalProvider::readBlock( int bandNo, QgsRectangle const &extent, int pixelWidth, int pixelHeight, void *data, QgsRasterBlockFeedback *feedback )
bool QgsGdalProvider::readBlock( int bandNo, QgsRectangle const &extent, int pixelWidth, int pixelHeight, void *data, QgsRasterBlockFeedback *feedback )
{
QMutexLocker locker( mpMutex );
if ( !initIfNeeded() )
return;
return false;

QgsDebugMsgLevel( "pixelWidth = " + QString::number( pixelWidth ), 5 );
QgsDebugMsgLevel( "pixelHeight = " + QString::number( pixelHeight ), 5 );
Expand Down Expand Up @@ -734,7 +746,7 @@ void QgsGdalProvider::readBlock( int bandNo, QgsRectangle const &extent, int pi
if ( rasterExtent.isEmpty() )
{
QgsDebugMsg( QStringLiteral( "draw request outside view extent." ) );
return;
return false;
}
QgsDebugMsgLevel( "extent: " + mExtent.toString(), 5 );
QgsDebugMsgLevel( "rasterExtent: " + rasterExtent.toString(), 5 );
Expand Down Expand Up @@ -878,7 +890,7 @@ void QgsGdalProvider::readBlock( int bandNo, QgsRectangle const &extent, int pi
if ( ! tmpBlock )
{
QgsDebugMsgLevel( QStringLiteral( "Couldn't allocate temporary buffer of %1 bytes" ).arg( dataSize * tmpWidth * tmpHeight ), 5 );
return;
return false;
}
GDALRasterBandH gdalBand = getBand( bandNo );
GDALDataType type = static_cast<GDALDataType>( mGdalDataType.at( bandNo - 1 ) );
Expand All @@ -894,7 +906,7 @@ void QgsGdalProvider::readBlock( int bandNo, QgsRectangle const &extent, int pi
{
QgsLogger::warning( "RasterIO error: " + QString::fromUtf8( CPLGetLastErrorMsg() ) );
qgsFree( tmpBlock );
return;
return false;
}

double tmpXRes = srcWidth * srcXRes / tmpWidth;
Expand Down Expand Up @@ -932,6 +944,7 @@ void QgsGdalProvider::readBlock( int bandNo, QgsRectangle const &extent, int pi
}

qgsFree( tmpBlock );
return true;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/providers/gdal/qgsgdalprovider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ class QgsGdalProvider : public QgsRasterDataProvider, QgsGdalProviderBase
// Reimplemented from QgsRasterDataProvider to bypass second resampling (more efficient for local file based sources)
QgsRasterBlock *block( int bandNo, const QgsRectangle &extent, int width, int height, QgsRasterBlockFeedback *feedback = nullptr ) override;

void readBlock( int bandNo, int xBlock, int yBlock, void *data ) override;
void readBlock( int bandNo, QgsRectangle const &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) override;
bool readBlock( int bandNo, int xBlock, int yBlock, void *data ) override;
bool readBlock( int bandNo, QgsRectangle const &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) override;
double bandScale( int bandNo ) const override;
double bandOffset( int bandNo ) const override;
QList<QgsColorRampShader::ColorRampItem> colorTable( int bandNo )const override;
Expand Down
16 changes: 9 additions & 7 deletions src/providers/wcs/qgswcsprovider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ void QgsWcsProvider::setQueryItem( QUrl &url, const QString &item, const QString
url.addQueryItem( item, value );
}

void QgsWcsProvider::readBlock( int bandNo, QgsRectangle const &viewExtent, int pixelWidth, int pixelHeight, void *block, QgsRasterBlockFeedback *feedback )
bool QgsWcsProvider::readBlock( int bandNo, QgsRectangle const &viewExtent, int pixelWidth, int pixelHeight, void *block, QgsRasterBlockFeedback *feedback )
{
// TODO: set block to null values, move that to function and call only if fails
memset( block, 0, pixelWidth * pixelHeight * QgsRasterBlock::typeSize( dataType( bandNo ) ) );
Expand All @@ -486,7 +486,7 @@ void QgsWcsProvider::readBlock( int bandNo, QgsRectangle const &viewExtent, int
// (higher level checks) but it is better to do check here as well
if ( !viewExtent.intersects( mCoverageExtent ) )
{
return;
return false;
}

// Can we reuse the previously cached coverage?
Expand Down Expand Up @@ -532,7 +532,7 @@ void QgsWcsProvider::readBlock( int bandNo, QgsRectangle const &viewExtent, int
// If it happens, it would be possible to rescale the portion we get
// to only part of the data block, but it is better to left it
// blank, so that the problem may be discovered in its origin.
return;
return false;
}
}

Expand All @@ -554,7 +554,7 @@ void QgsWcsProvider::readBlock( int bandNo, QgsRectangle const &viewExtent, int
if ( ! tmpData )
{
QgsDebugMsg( QStringLiteral( "Couldn't allocate memory of %1 bytes" ).arg( size ) );
return;
return false;
}
if ( GDALRasterIO( gdalBand, GF_Read, 0, 0, width, height, tmpData, width, height, ( GDALDataType ) mGdalDataType.at( bandNo - 1 ), 0, 0 ) != CE_None )
{
Expand Down Expand Up @@ -592,6 +592,7 @@ void QgsWcsProvider::readBlock( int bandNo, QgsRectangle const &viewExtent, int
QgsMessageLog::logMessage( tr( "Received coverage has wrong size %1 x %2 (expected %3 x %4)" ).arg( width ).arg( height ).arg( pixelWidth ).arg( pixelHeight ), tr( "WCS" ) );
}
}
return true;
}

void QgsWcsProvider::getCache( int bandNo, QgsRectangle const &viewExtent, int pixelWidth, int pixelHeight, QString crs, QgsRasterBlockFeedback *feedback ) const
Expand Down Expand Up @@ -795,12 +796,13 @@ void QgsWcsProvider::getCache( int bandNo, QgsRectangle const &viewExtent, int

// For stats only, maybe change QgsRasterDataProvider::bandStatistics() to
// use standard readBlock with extent
void QgsWcsProvider::readBlock( int bandNo, int xBlock, int yBlock, void *block )
bool QgsWcsProvider::readBlock( int bandNo, int xBlock, int yBlock, void *block )
{

QgsDebugMsg( QStringLiteral( "xBlock = %1 yBlock = %2" ).arg( xBlock ).arg( yBlock ) );

if ( !mHasSize ) return;
if ( !mHasSize )
return false;

double xRes = mCoverageExtent.width() / mWidth;
double yRes = mCoverageExtent.height() / mHeight;
Expand All @@ -815,7 +817,7 @@ void QgsWcsProvider::readBlock( int bandNo, int xBlock, int yBlock, void *block

QgsRectangle extent( xMin, yMin, xMax, yMax );

readBlock( bandNo, extent, mXBlockSize, mYBlockSize, block, nullptr );
return readBlock( bandNo, extent, mXBlockSize, mYBlockSize, block, nullptr );
}


Expand Down
4 changes: 2 additions & 2 deletions src/providers/wcs/qgswcsprovider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,9 @@ class QgsWcsProvider : public QgsRasterDataProvider, QgsGdalProviderBase

// TODO: Document this better.

void readBlock( int bandNo, QgsRectangle const &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) override;
bool readBlock( int bandNo, QgsRectangle const &viewExtent, int width, int height, void *data, QgsRasterBlockFeedback *feedback = nullptr ) override;

void readBlock( int bandNo, int xBlock, int yBlock, void *block ) override;
bool readBlock( int bandNo, int xBlock, int yBlock, void *block ) override;

//! Download cache
void getCache( int bandNo, QgsRectangle const &viewExtent, int width, int height, QString crs = QString(), QgsRasterBlockFeedback *feedback = nullptr ) const;
Expand Down
Loading

0 comments on commit 230c62f

Please sign in to comment.