Skip to content

Commit

Permalink
Don't store references to symbols in 3d symbol handlers
Browse files Browse the repository at this point in the history 
These references are to objects belonging to another thread, which
can cause crashes if the object on the main thread is deleted while
the handler is still active

Refs a asan report:

==677416==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070029bc278 at pc 0x7f95719ccc45 bp 0x7f94bdd7a310 sp 0x7f94bdd7a300
READ of size 4 at 0x6070029bc278 thread T36 (Thread (pooled))
    #0 0x7f95719ccc44 in QgsLine3DSymbol::extrusionHeight() const /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol.h:78
    #1 0x7f95674a7c01 in QgsBufferedLine3DSymbolHandler::processFeature(QgsFeature&, Qgs3DRenderContext const&) /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol_p.cpp:126
    #2 0x7f9567457045 in operator() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:97
    #3 0x7f95674597fd in runFunctor /usr/include/qt5/QtConcurrent/qtconcurrentstoredfunctioncall.h:70
    #4 0x7f956744acf7 in QtConcurrent::RunFunctionTask<void>::run() (/home/nyall/dev/build-QGIS-asan/output/lib/libqgis_3d.so.3.15.0+0x2adcf7)
    #5 0x7f956a275d59  (/usr/lib64/libQt5Core.so.5+0xc9d59)
    #6 0x7f956a27268f  (/usr/lib64/libQt5Core.so.5+0xc668f)
    #7 0x7f956a193431 in start_thread (/usr/lib64/libpthread.so.0+0x9431)
    #8 0x7f955ad59912 in __GI___clone (/usr/lib64/libc.so.6+0x101912)

0x6070029bc278 is located 56 bytes inside of 72-byte region [0x6070029bc240,0x6070029bc288)
freed by thread T0 here:
    #0 0x7f9572b86b87 in operator delete(void*) (/usr/lib64/libasan.so.6+0xb2b87)
    #1 0x7f956749c00b in QgsLine3DSymbol::~QgsLine3DSymbol() /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol.cpp:30
    #2 0x7f95719664ca in std::default_delete<QgsAbstract3DSymbol>::operator()(QgsAbstract3DSymbol*) const /usr/include/c++/10/bits/unique_ptr.h:85
    #3 0x7f95674428ce in std::unique_ptr<QgsAbstract3DSymbol, std::default_delete<QgsAbstract3DSymbol> >::~unique_ptr() /usr/include/c++/10/bits/unique_ptr.h:361
    #4 0x7f9567459eb6 in QgsVectorLayerChunkLoaderFactory::~QgsVectorLayerChunkLoaderFactory() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.h:53
    #5 0x7f9567459edd in QgsVectorLayerChunkLoaderFactory::~QgsVectorLayerChunkLoaderFactory() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.h:53
    #6 0x7f956746e4db in QgsChunkedEntity::~QgsChunkedEntity() /home/nyall/dev/qgis-asan/src/3d/chunks/qgschunkedentity_p.cpp:122
    #7 0x7f9567459358 in QgsVectorLayerChunkedEntity::~QgsVectorLayerChunkedEntity() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:164
    #8 0x7f9567459373 in QgsVectorLayerChunkedEntity::~QgsVectorLayerChunkedEntity() /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:168
    #9 0x7f956a42e960 in QObject::event(QEvent*) (/usr/lib64/libQt5Core.so.5+0x282960)
    #10 0x7f956ada2062 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib64/libQt5Widgets.so.5+0x172062)

previously allocated by thread T0 here:
    #0 0x7f9572b86067 in operator new(unsigned long) (/usr/lib64/libasan.so.6+0xb2067)
    #1 0x7f95674a3018 in qgis::_Unique_if<QgsLine3DSymbol>::_Single_object qgis::make_unique<QgsLine3DSymbol>() /home/nyall/dev/qgis-asan/src/core/qgis.h:425
    #2 0x7f956749c0b9 in QgsLine3DSymbol::clone() const /home/nyall/dev/qgis-asan/src/3d/symbols/qgsline3dsymbol.cpp:34
    #3 0x7f9567458b5f in QgsVectorLayerChunkLoaderFactory::QgsVectorLayerChunkLoaderFactory(Qgs3DMapSettings const&, QgsVectorLayer*, QgsAbstract3DSymbol*, int) /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:141
    #4 0x7f9567458d88 in QgsVectorLayerChunkedEntity::QgsVectorLayerChunkedEntity(QgsVectorLayer*, double, double, QgsVectorLayer3DTilingSettings const&, QgsAbstract3DSymbol*, Qgs3DMapSettings const&) /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayerchunkloader_p.cpp:159
    #5 0x7f956745558f in QgsVectorLayer3DRenderer::createEntity(Qgs3DMapSettings const&) const /home/nyall/dev/qgis-asan/src/3d/qgsvectorlayer3drenderer.cpp:76
    #6 0x7f95673a71c0 in Qgs3DMapScene::addLayerEntity(QgsMapLayer*) /home/nyall/dev/qgis-asan/src/3d/qgs3dmapscene.cpp:693
    #7 0x7f95673a5523 in Qgs3DMapScene::onLayerRenderer3DChanged() /home/nyall/dev/qgis-asan/src/3d/qgs3dmapscene.cpp:598
    #8 0x7f95673c6c33 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Qgs3DMapScene::*)()>::call(void (Qgs3DMapScene::*)(), Qgs3DMapScene*, void**) (/home/nyall/dev/build-QGIS-asan/output/lib/libqgis_3d.so.3.15.0+0x229c33)
    #9 0x7f95673c5468 in void QtPrivate::FunctionPointer<void (Qgs3DMapScene::*)()>::call<QtPrivate::List<>, void>(void (Qgs3DMapScene::*)(), Qgs3DMapScene*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185
    #10 0x7f95673c1748 in QtPrivate::QSlotObject<void (Qgs3DMapScene::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt5/QtCore/qobjectdefs_impl.h:418
    #11 0x7f956a435f75  (/usr/lib64/libQt5Core.so.5+0x289f75)
    #12 0x7f956252106c in QgsMapLayer::renderer3DChanged() src/core/qgis_core_autogen/EWIEGA46WW/moc_qgsmaplayer.cpp:702
    #13 0x7f9563917a12 in QgsMapLayer::setRenderer3D(QgsAbstract3DRenderer*) /home/nyall/dev/qgis-asan/src/core/qgsmaplayer.cpp:1813
    #14 0x7f95719aaf52 in QgsApp3DSymbolWidgetWithPreview::updatePreview(QgsAbstract3DSymbol*) /home/nyall/dev/qgis-asan/src/app/3d/qgsapp3dsymbolwidget.cpp:243
    #15 0x7f95719a97af in operator() /home/nyall/dev/qgis-asan/src/app/3d/qgsapp3dsymbolwidget.cpp:175
    #16 0x7f95719ac2dd in call /usr/include/qt5/QtCore/qobjectdefs_impl.h:146
    #17 0x7f95719ac24f in call<QtPrivate::List<>, void> /usr/include/qt5/QtCore/qobjectdefs_impl.h:256
    #18 0x7f95719ac21e in impl /usr/include/qt5/QtCore/qobjectdefs_impl.h:443
    #19 0x7f956a435f75  (/usr/lib64/libQt5Core.so.5+0x289f75)
    #20 0x7f95708d15ec in QgsApp3DSymbolWidget::widgetChanged() src/app/qgis_app_autogen/6LADBHSVD5/moc_qgsapp3dsymbolwidget.cpp:144
    #21 0x7f95719ae1dd in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (QgsApp3DSymbolWidget::*)()>::call(void (QgsApp3DSymbolWidget::*)(), QgsApp3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:152
    #22 0x7f95719add9c in void QtPrivate::FunctionPointer<void (QgsApp3DSymbolWidget::*)()>::call<QtPrivate::List<>, void>(void (QgsApp3DSymbolWidget::*)(), QgsApp3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185
    #23 0x7f95719adbfa in QtPrivate::QSlotObject<void (QgsApp3DSymbolWidget::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt5/QtCore/qobjectdefs_impl.h:418
    #24 0x7f956a435f75  (/usr/lib64/libQt5Core.so.5+0x289f75)
    #25 0x7f956c91c63e in Qgs3DSymbolWidget::changed() src/gui/qgis_gui_autogen/EWIEGA46WW/moc_qgs3dsymbolwidget.cpp:131
    #26 0x7f95719ce2ad in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Qgs3DSymbolWidget::*)()>::call(void (Qgs3DSymbolWidget::*)(), Qgs3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:152
    #27 0x7f95719ce014 in void QtPrivate::FunctionPointer<void (Qgs3DSymbolWidget::*)()>::call<QtPrivate::List<>, void>(void (Qgs3DSymbolWidget::*)(), Qgs3DSymbolWidget*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185
    #28 0x7f95719cdbfc in QtPrivate::QSlotObject<void (Qgs3DSymbolWidget::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt5/QtCore/qobjectdefs_impl.h:418
    #29 0x7f956a435f75  (/usr/lib64/libQt5Core.so.5+0x289f75)
  • Loading branch information
@nyalldawson
nyalldawson committed Aug 21, 2020
1 parent d411d66 commit a27930c
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 95 deletions.
53 changes: 28 additions & 25 deletions src/3d/symbols/qgsline3dsymbol_p.cpp
View file
Expand Up @@ -45,8 +45,9 @@
class QgsBufferedLine3DSymbolHandler : public QgsFeature3DHandler
{
public:
QgsBufferedLine3DSymbolHandler( const QgsLine3DSymbol &symbol, const QgsFeatureIds &selectedIds )
: mSymbol( symbol ), mSelectedIds( selectedIds ) {}
QgsBufferedLine3DSymbolHandler( const QgsLine3DSymbol *symbol, const QgsFeatureIds &selectedIds )
: mSymbol( static_cast< QgsLine3DSymbol *>( symbol->clone() ) )
, mSelectedIds( selectedIds ) {}

bool prepare( const Qgs3DRenderContext &context, QSet<QString> &attributeNames ) override;
void processFeature( QgsFeature &feature, const Qgs3DRenderContext &context ) override;
Expand All @@ -67,7 +68,7 @@ class QgsBufferedLine3DSymbolHandler : public QgsFeature3DHandler
void makeEntity( Qt3DCore::QEntity *parent, const Qgs3DRenderContext &context, LineData &out, bool selected );

// input specific for this class
const QgsLine3DSymbol &mSymbol;
std::unique_ptr< QgsLine3DSymbol > mSymbol;
// inputs - generic
QgsFeatureIds mSelectedIds;

Expand All @@ -82,7 +83,7 @@ bool QgsBufferedLine3DSymbolHandler::prepare( const Qgs3DRenderContext &context,
{
Q_UNUSED( attributeNames )

const QgsPhongTexturedMaterialSettings *texturedMaterialSettings = dynamic_cast< const QgsPhongTexturedMaterialSettings * >( mSymbol.material() );
const QgsPhongTexturedMaterialSettings *texturedMaterialSettings = dynamic_cast< const QgsPhongTexturedMaterialSettings * >( mSymbol->material() );

outNormal.tessellator.reset( new QgsTessellator( context.map().origin().x(), context.map().origin().y(), true,
false, false, false, texturedMaterialSettings ? texturedMaterialSettings->requiresTextureCoordinates() : false,
Expand Down Expand Up @@ -118,28 +119,28 @@ void QgsBufferedLine3DSymbolHandler::processFeature( QgsFeature &f, const Qgs3DR
const double mitreLimit = 0;

QgsGeos engine( g );
QgsAbstractGeometry *buffered = engine.buffer( mSymbol.width() / 2., nSegments, endCapStyle, joinStyle, mitreLimit ); // factory
QgsAbstractGeometry *buffered = engine.buffer( mSymbol->width() / 2., nSegments, endCapStyle, joinStyle, mitreLimit ); // factory

if ( QgsWkbTypes::flatType( buffered->wkbType() ) == QgsWkbTypes::Polygon )
{
QgsPolygon *polyBuffered = static_cast<QgsPolygon *>( buffered );
processPolygon( polyBuffered, f.id(), mSymbol.height(), mSymbol.extrusionHeight(), context, out );
processPolygon( polyBuffered, f.id(), mSymbol->height(), mSymbol->extrusionHeight(), context, out );
}
else if ( QgsWkbTypes::flatType( buffered->wkbType() ) == QgsWkbTypes::MultiPolygon )
{
QgsMultiPolygon *mpolyBuffered = static_cast<QgsMultiPolygon *>( buffered );
for ( int i = 0; i < mpolyBuffered->numGeometries(); ++i )
{
QgsPolygon *polyBuffered = static_cast<QgsPolygon *>( mpolyBuffered->polygonN( i ) )->clone(); // need to clone individual geometry parts
processPolygon( polyBuffered, f.id(), mSymbol.height(), mSymbol.extrusionHeight(), context, out );
processPolygon( polyBuffered, f.id(), mSymbol->height(), mSymbol->extrusionHeight(), context, out );
}
delete buffered;
}
}

void QgsBufferedLine3DSymbolHandler::processPolygon( QgsPolygon *polyBuffered, QgsFeatureId fid, float height, float extrusionHeight, const Qgs3DRenderContext &context, LineData &out )
{
Qgs3DUtils::clampAltitudes( polyBuffered, mSymbol.altitudeClamping(), mSymbol.altitudeBinding(), height, context.map() );
Qgs3DUtils::clampAltitudes( polyBuffered, mSymbol->altitudeClamping(), mSymbol->altitudeBinding(), height, context.map() );

Q_ASSERT( out.tessellator->dataVerticesCount() % 3 == 0 );
uint startingTriangleIndex = static_cast<uint>( out.tessellator->dataVerticesCount() / 3 );
Expand Down Expand Up @@ -168,13 +169,13 @@ void QgsBufferedLine3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, cons
QgsMaterialContext materialContext;
materialContext.setIsSelected( selected );
materialContext.setSelectionColor( context.map().selectionColor() );
Qt3DRender::QMaterial *mat = mSymbol.material()->toMaterial( QgsMaterialSettingsRenderingTechnique::Triangles, materialContext );
Qt3DRender::QMaterial *mat = mSymbol->material()->toMaterial( QgsMaterialSettingsRenderingTechnique::Triangles, materialContext );

// extract vertex buffer data from tessellator
QByteArray data( ( const char * )out.tessellator->data().constData(), out.tessellator->data().count() * sizeof( float ) );
int nVerts = data.count() / out.tessellator->stride();

const QgsPhongTexturedMaterialSettings *texturedMaterialSettings = dynamic_cast< const QgsPhongTexturedMaterialSettings * >( mSymbol.material() );
const QgsPhongTexturedMaterialSettings *texturedMaterialSettings = dynamic_cast< const QgsPhongTexturedMaterialSettings * >( mSymbol->material() );

QgsTessellatedPolygonGeometry *geometry = new QgsTessellatedPolygonGeometry( true, false, false,
texturedMaterialSettings ? texturedMaterialSettings->requiresTextureCoordinates() : false );
Expand Down Expand Up @@ -203,8 +204,9 @@ void QgsBufferedLine3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, cons
class QgsSimpleLine3DSymbolHandler : public QgsFeature3DHandler
{
public:
QgsSimpleLine3DSymbolHandler( const QgsLine3DSymbol &symbol, const QgsFeatureIds &selectedIds )
: mSymbol( symbol ), mSelectedIds( selectedIds )
QgsSimpleLine3DSymbolHandler( const QgsLine3DSymbol *symbol, const QgsFeatureIds &selectedIds )
: mSymbol( static_cast< QgsLine3DSymbol *>( symbol->clone() ) )
, mSelectedIds( selectedIds )
{
}

Expand All @@ -218,7 +220,7 @@ class QgsSimpleLine3DSymbolHandler : public QgsFeature3DHandler
Qt3DExtras::QPhongMaterial *material( const QgsLine3DSymbol &symbol ) const;

// input specific for this class
const QgsLine3DSymbol &mSymbol;
std::unique_ptr< QgsLine3DSymbol > mSymbol;
// inputs - generic
QgsFeatureIds mSelectedIds;

Expand All @@ -233,8 +235,8 @@ bool QgsSimpleLine3DSymbolHandler::prepare( const Qgs3DRenderContext &context, Q
{
Q_UNUSED( attributeNames )

outNormal.init( mSymbol.altitudeClamping(), mSymbol.altitudeBinding(), mSymbol.height(), &context.map() );
outSelected.init( mSymbol.altitudeClamping(), mSymbol.altitudeBinding(), mSymbol.height(), &context.map() );
outNormal.init( mSymbol->altitudeClamping(), mSymbol->altitudeBinding(), mSymbol->height(), &context.map() );
outSelected.init( mSymbol->altitudeClamping(), mSymbol->altitudeBinding(), mSymbol->height(), &context.map() );

return true;
}
Expand Down Expand Up @@ -284,7 +286,7 @@ void QgsSimpleLine3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, const
QgsMaterialContext materialContext;
materialContext.setIsSelected( selected );
materialContext.setSelectionColor( context.map().selectionColor() );
Qt3DRender::QMaterial *mat = mSymbol.material()->toMaterial( QgsMaterialSettingsRenderingTechnique::Lines, materialContext );
Qt3DRender::QMaterial *mat = mSymbol->material()->toMaterial( QgsMaterialSettingsRenderingTechnique::Lines, materialContext );

// geometry renderer

Expand Down Expand Up @@ -313,8 +315,9 @@ void QgsSimpleLine3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, const
class QgsThickLine3DSymbolHandler : public QgsFeature3DHandler
{
public:
QgsThickLine3DSymbolHandler( const QgsLine3DSymbol &symbol, const QgsFeatureIds &selectedIds )
: mSymbol( symbol ), mSelectedIds( selectedIds )
QgsThickLine3DSymbolHandler( const QgsLine3DSymbol *symbol, const QgsFeatureIds &selectedIds )
: mSymbol( static_cast< QgsLine3DSymbol * >( symbol->clone() ) )
, mSelectedIds( selectedIds )
{
}

Expand All @@ -329,7 +332,7 @@ class QgsThickLine3DSymbolHandler : public QgsFeature3DHandler
Qt3DExtras::QPhongMaterial *material( const QgsLine3DSymbol &symbol ) const;

// input specific for this class
const QgsLine3DSymbol &mSymbol;
std::unique_ptr< QgsLine3DSymbol > mSymbol;
// inputs - generic
QgsFeatureIds mSelectedIds;

Expand All @@ -346,8 +349,8 @@ bool QgsThickLine3DSymbolHandler::prepare( const Qgs3DRenderContext &context, QS

outNormal.withAdjacency = true;
outSelected.withAdjacency = true;
outNormal.init( mSymbol.altitudeClamping(), mSymbol.altitudeBinding(), mSymbol.height(), &context.map() );
outSelected.init( mSymbol.altitudeClamping(), mSymbol.altitudeBinding(), mSymbol.height(), &context.map() );
outNormal.init( mSymbol->altitudeClamping(), mSymbol->altitudeBinding(), mSymbol->height(), &context.map() );
outSelected.init( mSymbol->altitudeClamping(), mSymbol->altitudeBinding(), mSymbol->height(), &context.map() );

return true;
}
Expand Down Expand Up @@ -396,15 +399,15 @@ void QgsThickLine3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, const Q
QgsMaterialContext materialContext;
materialContext.setIsSelected( selected );
materialContext.setSelectionColor( context.map().selectionColor() );
Qt3DRender::QMaterial *mat = mSymbol.material()->toMaterial( QgsMaterialSettingsRenderingTechnique::Lines, materialContext );
Qt3DRender::QMaterial *mat = mSymbol->material()->toMaterial( QgsMaterialSettingsRenderingTechnique::Lines, materialContext );
if ( !mat )
{
QgsSimpleLineMaterialSettings defaultMaterial;
mat = defaultMaterial.toMaterial( QgsMaterialSettingsRenderingTechnique::Lines, materialContext );
}

if ( QgsLineMaterial *lineMaterial = dynamic_cast< QgsLineMaterial * >( mat ) )
lineMaterial->setLineWidth( mSymbol.width() );
lineMaterial->setLineWidth( mSymbol->width() );

Qt3DCore::QEntity *entity = new Qt3DCore::QEntity;

Expand Down Expand Up @@ -436,10 +439,10 @@ namespace Qgs3DSymbolImpl
return nullptr;

if ( lineSymbol->renderAsSimpleLines() )
return new QgsThickLine3DSymbolHandler( *lineSymbol, layer->selectedFeatureIds() );
return new QgsThickLine3DSymbolHandler( lineSymbol, layer->selectedFeatureIds() );
//return new QgsSimpleLine3DSymbolHandler( symbol, layer->selectedFeatureIds() );
else
return new QgsBufferedLine3DSymbolHandler( *lineSymbol, layer->selectedFeatureIds() );
return new QgsBufferedLine3DSymbolHandler( lineSymbol, layer->selectedFeatureIds() );
}

Qt3DCore::QEntity *entityForLine3DSymbol( const Qgs3DMapSettings &map, QgsVectorLayer *layer, const QgsLine3DSymbol &symbol )
Expand Down

0 comments on commit a27930c

Please sign in to comment.