If an error occurs while reading raster block data, don't return

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 (cherry picked from commit 230c62f)
qgis · May 23, 2019 · 76b29dc · 76b29dc
1 parent fa2ae2f
commit 76b29dc
Show file tree

Hide file tree

Showing 10 changed files with 77 additions and 52 deletions.
diff --git a/src/core/raster/qgsrasterdataprovider.cpp b/src/core/raster/qgsrasterdataprovider.cpp
@@ -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 ) );
@@ -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
@@ -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;
@@ -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
@@ -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 );
 
@@ -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;
@@ -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()

diff --git a/src/core/raster/qgsrasterdataprovider.h b/src/core/raster/qgsrasterdataprovider.h
@@ -538,15 +538,15 @@ class CORE_EXPORT QgsRasterDataProvider : public QgsDataProvider, public QgsRast
      * Read block of data
      * \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
      * \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;

diff --git a/src/providers/arcgisrest/qgsamsprovider.cpp b/src/providers/arcgisrest/qgsamsprovider.cpp
@@ -454,7 +454,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
 
@@ -463,9 +463,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

diff --git a/src/providers/arcgisrest/qgsamsprovider.h b/src/providers/arcgisrest/qgsamsprovider.h
@@ -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 );
 

diff --git a/src/providers/gdal/qgsgdalprovider.cpp b/src/providers/gdal/qgsgdalprovider.cpp
@@ -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???
@@ -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 );
@@ -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 );
@@ -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 ) );
@@ -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;
@@ -932,6 +944,7 @@ void QgsGdalProvider::readBlock( int bandNo, QgsRectangle  const &extent, int pi
   }
 
   qgsFree( tmpBlock );
+  return true;
 }
 
 /**

diff --git a/src/providers/gdal/qgsgdalprovider.h b/src/providers/gdal/qgsgdalprovider.h
@@ -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;

diff --git a/src/providers/wcs/qgswcsprovider.cpp b/src/providers/wcs/qgswcsprovider.cpp
@@ -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 ) ) );
@@ -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?
@@ -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;
       }
     }
 
@@ -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 )
       {
@@ -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
@@ -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;
@@ -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 );
 }
 
 

diff --git a/src/providers/wcs/qgswcsprovider.h b/src/providers/wcs/qgswcsprovider.h
@@ -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;

diff --git a/src/providers/wms/qgswmsprovider.cpp b/src/providers/wms/qgswmsprovider.cpp
@@ -883,15 +883,15 @@ QImage *QgsWmsProvider::draw( QgsRectangle const &viewExtent, int pixelWidth, in
   return image;
 }
 
-void QgsWmsProvider::readBlock( int bandNo, QgsRectangle  const &viewExtent, int pixelWidth, int pixelHeight, void *block, QgsRasterBlockFeedback *feedback )
+bool QgsWmsProvider::readBlock( int bandNo, QgsRectangle  const &viewExtent, int pixelWidth, int pixelHeight, void *block, QgsRasterBlockFeedback *feedback )
 {
   Q_UNUSED( bandNo );
   // TODO: optimize to avoid writing to QImage
-  QImage *image = draw( viewExtent, pixelWidth, pixelHeight, feedback );
+  std::unique_ptr< QImage > image( draw( viewExtent, pixelWidth, pixelHeight, feedback ) );
   if ( !image )   // should not happen
   {
     QgsMessageLog::logMessage( tr( "image is NULL" ), tr( "WMS" ) );
-    return;
+    return false;
   }
 
   QgsDebugMsg( QStringLiteral( "image height = %1 bytesPerLine = %2" ).arg( image->height() ) . arg( image->bytesPerLine() ) );
@@ -900,18 +900,20 @@ void QgsWmsProvider::readBlock( int bandNo, QgsRectangle  const &viewExtent, int
   if ( myExpectedSize != myImageSize )   // should not happen
   {
     QgsMessageLog::logMessage( tr( "unexpected image size" ), tr( "WMS" ) );
-    delete image;
-    return;
+    return false;
   }
 
   uchar *ptr = image->bits();
   if ( ptr )
   {
     // If image is too large, ptr can be NULL
     memcpy( block, ptr, myExpectedSize );
+    return true;
+  }
+  else
+  {
+    return false;
   }
-
-  delete image;
 }
 
 QUrl QgsWmsProvider::createRequestUrlWMS( const QgsRectangle &viewExtent, int pixelWidth, int pixelHeight )