From 8243598ff0a6f077d046e4eac0a00c28783111de Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Mon, 20 Nov 2017 10:24:55 +1000 Subject: [PATCH] Even more memory safety for ogr layers --- src/providers/ogr/qgsogrprovider.cpp | 60 ++++++++++++---------------- src/providers/ogr/qgsogrprovider.h | 48 ++++++++++++---------- 2 files changed, 51 insertions(+), 57 deletions(-) diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index 5030d72d1ec2..f1946a05e9c6 100644 --- a/src/providers/ogr/qgsogrprovider.cpp +++ b/src/providers/ogr/qgsogrprovider.cpp @@ -192,18 +192,17 @@ void QgsOgrProvider::repack() { QgsMessageLog::logMessage( tr( "Possible corruption after REPACK detected. %1 still exists. This may point to a permission or locking problem of the original DBF." ).arg( packedDbf ), tr( "OGR" ), QgsMessageLog::CRITICAL ); - if ( mOgrLayer != mOgrOrigLayer ) - QgsOgrProviderUtils::release( mOgrLayer ); - QgsOgrProviderUtils::release( mOgrOrigLayer ); + mOgrSqlLayer.reset(); + mOgrOrigLayer.reset(); QString errCause; if ( mLayerName.isNull() ) { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerIndex, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerIndex, errCause ); } else { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerName, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerName, errCause ); } if ( !mOgrOrigLayer ) @@ -212,7 +211,7 @@ void QgsOgrProvider::repack() mValid = false; } - mOgrLayer = mOgrOrigLayer; + mOgrLayer = mOgrOrigLayer.get(); } } @@ -526,17 +525,14 @@ bool QgsOgrProvider::setSubsetString( const QString &theSQL, bool updateFeatureC pushError( tr( "OGR[%1] error %2: %3" ).arg( CPLGetLastErrorType() ).arg( CPLGetLastErrorNo() ).arg( CPLGetLastErrorMsg() ) ); return false; } - QgsOgrLayerUniquePtr newLayer = QgsOgrProviderUtils::getSqlLayer( mOgrOrigLayer, subsetLayerH, theSQL ); - Q_ASSERT( newLayer ); - if ( mOgrLayer != mOgrOrigLayer ) - QgsOgrProviderUtils::release( mOgrLayer ); - mOgrLayer = newLayer.release(); + mOgrSqlLayer = QgsOgrProviderUtils::getSqlLayer( mOgrOrigLayer.get(), subsetLayerH, theSQL ); + Q_ASSERT( mOgrSqlLayer.get() ); + mOgrLayer = mOgrSqlLayer.get(); } else { - if ( mOgrLayer != mOgrOrigLayer ) - QgsOgrProviderUtils::release( mOgrLayer ); - mOgrLayer = mOgrOrigLayer; + mOgrSqlLayer.reset(); + mOgrLayer = mOgrOrigLayer.get(); } mSubsetString = theSQL; @@ -1138,7 +1134,7 @@ QgsRectangle QgsOgrProvider::extent() const mExtent->MaxY = -std::numeric_limits::max(); // TODO: This can be expensive, do we really need it! - if ( mOgrLayer == mOgrOrigLayer ) + if ( mOgrLayer == mOgrOrigLayer.get() ) { mOgrLayer->GetExtent( mExtent.get(), true ); } @@ -1215,8 +1211,7 @@ size_t QgsOgrProvider::layerCount() const if ( !mValid ) return 0; return mOgrLayer->GetLayerCount(); -} // QgsOgrProvider::layerCount() - +} /** * Return the feature type @@ -3787,7 +3782,7 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, GDALDatasetH ds void QgsOgrProvider::open( OpenMode mode ) { bool openReadOnly = false; - Q_ASSERT( !mOgrLayer ); + Q_ASSERT( !mOgrSqlLayer ); Q_ASSERT( !mOgrOrigLayer ); // Try to open using VSIFileHandler @@ -3836,11 +3831,11 @@ void QgsOgrProvider::open( OpenMode mode ) // has precedence over the layerid if both are given. if ( !mLayerName.isNull() ) { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerName, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerName, errCause ); } else { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerIndex, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerIndex, errCause ); } } @@ -3861,11 +3856,11 @@ void QgsOgrProvider::open( OpenMode mode ) // try to open read-only if ( !mLayerName.isNull() ) { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause ); } else { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause ); } } @@ -3875,7 +3870,7 @@ void QgsOgrProvider::open( OpenMode mode ) QgsDebugMsg( "OGR opened using Driver " + mGDALDriverName ); - mOgrLayer = mOgrOrigLayer; + mOgrLayer = mOgrOrigLayer.get(); // check that the initial encoding setting is fit for this layer setEncoding( encoding() ); @@ -3912,8 +3907,8 @@ void QgsOgrProvider::open( OpenMode mode ) if ( mValid && mode == OpenModeInitial && mWriteAccess && ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) || mGDALDriverName == QLatin1String( "MapInfo File" ) ) ) { - QgsOgrProviderUtils::release( mOgrOrigLayer ); - mOgrLayer = mOgrOrigLayer = nullptr; + mOgrOrigLayer.reset(); + mOgrLayer = nullptr; mValid = false; // In the case where we deal with a shapefile, it is possible that it has @@ -3929,15 +3924,15 @@ void QgsOgrProvider::open( OpenMode mode ) // try to open read-only if ( !mLayerName.isNull() ) { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause ); } else { - mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause ).release(); + mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause ); } mWriteAccess = false; - mOgrLayer = mOgrOrigLayer; + mOgrLayer = mOgrOrigLayer.get(); if ( mOgrLayer ) { mValid = true; @@ -3964,14 +3959,9 @@ void QgsOgrProvider::open( OpenMode mode ) void QgsOgrProvider::close() { - if ( mOgrLayer != mOgrOrigLayer ) - { - QgsOgrProviderUtils::release( mOgrLayer ); - } - - QgsOgrProviderUtils::release( mOgrOrigLayer ); + mOgrSqlLayer.reset(); + mOgrOrigLayer.reset(); mOgrLayer = nullptr; - mOgrOrigLayer = nullptr; mValid = false; setProperty( "_debug_open_mode", "invalid" ); diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h index cc65f0be3510..0c025762918e 100644 --- a/src/providers/ogr/qgsogrprovider.h +++ b/src/providers/ogr/qgsogrprovider.h @@ -34,6 +34,24 @@ class QgsOgrFeatureIterator; class QgsOgrLayer; +/** + * Releases a QgsOgrLayer + */ +struct QgsOgrLayerReleaser +{ + + /** + * Releases a QgsOgrLayer \a layer. + */ + void operator()( QgsOgrLayer *layer ); + +}; + +/** + * Scoped QgsOgrLayer. + */ +using QgsOgrLayerUniquePtr = std::unique_ptr< QgsOgrLayer, QgsOgrLayerReleaser>; + /** \class QgsOgrProvider \brief Data provider for OGR datasources @@ -206,11 +224,17 @@ class QgsOgrProvider : public QgsVectorDataProvider in the method QgsOgrProvider::extent(). The purpose is to prevent a memory leak*/ mutable QgsRectangle mExtentRect; - //! Current working layer (might be a SQL result layer if mSubsetString is set) + /** + * Current working layer - will point to either mOgrSqlLayer or mOgrOrigLayer depending + * on whether a subset string is set + */ QgsOgrLayer *mOgrLayer = nullptr; + //! SQL result layer, used if a subset string is set + QgsOgrLayerUniquePtr mOgrSqlLayer; + //! Original layer (not a SQL result layer) - QgsOgrLayer *mOgrOrigLayer = nullptr; + QgsOgrLayerUniquePtr mOgrOrigLayer; //! path to filename QString mFilePath; @@ -285,26 +309,6 @@ class QgsOgrProvider : public QgsVectorDataProvider }; -class QgsOgrLayer; - -/** - * Releases a QgsOgrLayer - */ -struct QgsOgrLayerReleaser -{ - - /** - * Releases a QgsOgrLayer \a layer. - */ - void operator()( QgsOgrLayer *layer ); - -}; - -/** - * Scoped QgsOgrLayer. - */ -using QgsOgrLayerUniquePtr = std::unique_ptr< QgsOgrLayer, QgsOgrLayerReleaser>; - /** \class QgsOgrProviderUtils \brief Utility class with static methods