Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix crash when attempting to render multipolygon with missing exterio…
…r ring

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
  • Loading branch information
nyalldawson committed Nov 1, 2017
1 parent 6418a83 commit eea155d
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/core/qgsvectorlayerrenderer.cpp
Expand Up @@ -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 );
Expand Down
3 changes: 3 additions & 0 deletions src/core/symbology/qgssymbol.cpp
Expand Up @@ -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 );

Expand Down
91 changes: 86 additions & 5 deletions tests/src/core/testqgsrenderers.cpp
Expand Up @@ -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"

Expand All @@ -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;
Expand Down Expand Up @@ -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
//
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit eea155d

Please sign in to comment.