Fix crash when attempting to render multipolygon with missing exterior ring

nyalldawson · nyalldawson · commit eea155d6e28b · 2017-11-01T21:48:20.000+11:00
This commit fixes a possible crash when the vector layer renderer attempts to render a multipolygon containing a polygon without an exterior ring. The underlying cause of the creation of this invalid geometry is deeper, but this commit hardens the renderer and makes it more robust for handling bad geometries. Fixes #17365
diff --git a/src/core/qgsvectorlayerrenderer.cpp b/src/core/qgsvectorlayerrenderer.cpp
@@ -271,7 +271,7 @@ void QgsVectorLayerRenderer::drawRenderer( QgsFeatureIterator &fit )
         break;
       }
 
-      if ( !fet.hasGeometry() )
+      if ( !fet.hasGeometry() || fet.geometry().isEmpty() )
         continue; // skip features without geometry
 
       mContext.expressionContext().setFeature( fet );
diff --git a/src/core/symbology/qgssymbol.cpp b/src/core/symbology/qgssymbol.cpp
@@ -914,6 +914,9 @@ void QgsSymbol::renderFeature( const QgsFeature &feature, QgsRenderContext &cont
 
           context.setGeometry( geomCollection.geometryN( i ) );
           const QgsPolygon &polygon = dynamic_cast<const QgsPolygon &>( *geomCollection.geometryN( i ) );
+          if ( !polygon.exteriorRing() )
+            break;
+
           _getPolygon( pts, holes, context, polygon, !tileMapRendering && clipFeaturesToExtent() );
           static_cast<QgsFillSymbol *>( this )->renderPolygon( pts, ( !holes.isEmpty() ? &holes : nullptr ), &feature, context, layer, selected );
 
diff --git a/tests/src/core/testqgsrenderers.cpp b/tests/src/core/testqgsrenderers.cpp
@@ -22,11 +22,16 @@
 #include <QDesktopServices>
 
 //qgis includes...
-#include <qgsmaplayer.h>
-#include <qgsvectorlayer.h>
-#include <qgsapplication.h>
-#include <qgsproviderregistry.h>
-#include <qgsproject.h>
+#include "qgsmaplayer.h"
+#include "qgsvectorlayer.h"
+#include "qgsapplication.h"
+#include "qgsproviderregistry.h"
+#include "qgsproject.h"
+#include "qgsmultipolygon.h"
+#include "qgspolygon.h"
+#include "qgslinestring.h"
+#include "qgsmultilinestring.h"
+#include "qgsmultipoint.h"
 //qgis test includes
 #include "qgsmultirenderchecker.h"
 
@@ -52,13 +57,15 @@ class TestQgsRenderers : public QObject
     void cleanup() {} // will be called after every testfunction.
 
     void singleSymbol();
+    void emptyGeometry();
 //    void uniqueValue();
 //    void graduatedSymbol();
 //    void continuousSymbol();
   private:
     bool mTestHasError =  false ;
     bool setQml( const QString &type ); //uniquevalue / continuous / single /
     bool imageCheck( const QString &type ); //as above
+    bool checkEmptyRender( const QString &name, QgsVectorLayer *layer );
     QgsMapSettings *mMapSettings = nullptr;
     QgsMapLayer *mpPointsLayer = nullptr;
     QgsMapLayer *mpLinesLayer = nullptr;
@@ -147,6 +154,80 @@ void TestQgsRenderers::singleSymbol()
   QVERIFY( imageCheck( "single" ) );
 }
 
+void TestQgsRenderers::emptyGeometry()
+{
+  // test rendering an empty geometry
+  // note - this test is of limited use, because the features with empty geometries should not be rendered
+  // by the feature iterator given that renderer uses a filter rect on the request. It's placed here
+  // for manual testing by commenting out the part of the renderer which places the filterrect on the
+  // layer's feature request. The purpose of this test is to ensure that we do not crash for empty geometries,
+  // as it's possible that malformed providers OR bugs in underlying libraries will still return empty geometries
+  // even when a filter rect request was made, and we shouldn't crash for these.
+  QgsVectorLayer *vl = new QgsVectorLayer( QStringLiteral( "MultiPolygon?crs=epsg:4326&field=pk:int&field=col1:string" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) );
+  QVERIFY( vl->isValid() );
+  QgsProject::instance()->addMapLayer( vl );
+
+  QgsFeature f;
+  std::unique_ptr< QgsMultiPolygon > mp = qgis::make_unique< QgsMultiPolygon >();
+  mp->addGeometry( new QgsPolygon() );
+  f.setGeometry( QgsGeometry( std::move( mp ) ) );
+  QVERIFY( vl->dataProvider()->addFeature( f ) );
+  QVERIFY( checkEmptyRender( "Multipolygon", vl ) );
+
+  // polygon
+  vl = new QgsVectorLayer( QStringLiteral( "Polygon?crs=epsg:4326&field=pk:int&field=col1:string" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) );
+  QVERIFY( vl->isValid() );
+  QgsProject::instance()->addMapLayer( vl );
+  f.setGeometry( QgsGeometry( new QgsPolygon() ) );
+  QVERIFY( vl->dataProvider()->addFeature( f ) );
+  QVERIFY( checkEmptyRender( "Polygon", vl ) );
+
+  // linestring
+  vl = new QgsVectorLayer( QStringLiteral( "LineString?crs=epsg:4326&field=pk:int&field=col1:string" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) );
+  QVERIFY( vl->isValid() );
+  QgsProject::instance()->addMapLayer( vl );
+  f.setGeometry( QgsGeometry( new QgsLineString() ) );
+  QVERIFY( vl->dataProvider()->addFeature( f ) );
+  QVERIFY( checkEmptyRender( "LineString", vl ) );
+
+  // multilinestring
+  vl = new QgsVectorLayer( QStringLiteral( "MultiLineString?crs=epsg:4326&field=pk:int&field=col1:string" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) );
+  QVERIFY( vl->isValid() );
+  QgsProject::instance()->addMapLayer( vl );
+  std::unique_ptr< QgsMultiLineString > mls = qgis::make_unique< QgsMultiLineString >();
+  mls->addGeometry( new QgsLineString() );
+  f.setGeometry( QgsGeometry( std::move( mls ) ) );
+  QVERIFY( vl->dataProvider()->addFeature( f ) );
+  QVERIFY( checkEmptyRender( "MultiLineString", vl ) );
+
+  // multipoint
+  vl = new QgsVectorLayer( QStringLiteral( "MultiPoint?crs=epsg:4326&field=pk:int&field=col1:string" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) );
+  QVERIFY( vl->isValid() );
+  QgsProject::instance()->addMapLayer( vl );
+  std::unique_ptr< QgsMultiPoint > mlp = qgis::make_unique< QgsMultiPoint >();
+  f.setGeometry( QgsGeometry( std::move( mlp ) ) );
+  QVERIFY( vl->dataProvider()->addFeature( f ) );
+  QVERIFY( checkEmptyRender( "MultiPoint", vl ) );
+}
+
+bool TestQgsRenderers::checkEmptyRender( const QString &testName, QgsVectorLayer *layer )
+{
+  QgsMapSettings ms;
+  QgsRectangle extent( -180, -90, 180, 90 );
+  ms.setExtent( extent );
+  ms.setFlag( QgsMapSettings::ForceVectorOutput );
+  ms.setOutputDpi( 96 );
+  ms.setLayers( QList< QgsMapLayer * >() << layer );
+  QgsMultiRenderChecker myChecker;
+  myChecker.setControlName( "expected_emptygeometry" );
+  myChecker.setMapSettings( ms );
+  myChecker.setControlPathPrefix( "map_renderer" );
+  myChecker.setColorTolerance( 15 );
+  bool myResultFlag = myChecker.runTest( testName, 200 );
+  mReport += myChecker.report();
+  return myResultFlag;
+}
+
 //
 // Private helper functions not called directly by CTest
 //
diff --git a/tests/testdata/control_images/map_renderer/expected_emptygeometry/expected_emptygeometry.png b/tests/testdata/control_images/map_renderer/expected_emptygeometry/expected_emptygeometry.png

Original file line number	Diff line number	Diff line change
`@@ -271,7 +271,7 @@ void QgsVectorLayerRenderer::drawRenderer( QgsFeatureIterator &fit )`
`271`	`271`	`break;`
`272`	`272`	`}`
`273`	`273`
`274`		`- if ( !fet.hasGeometry() )`
	`274`	`+ if ( !fet.hasGeometry() \|\| fet.geometry().isEmpty() )`
`275`	`275`	`continue; // skip features without geometry`
`276`	`276`
`277`	`277`	`mContext.expressionContext().setFeature( fet );`