From f6cb8b1af8f15a06898c5c71f81c64779d9478f6 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 20 Sep 2016 15:32:17 +0200 Subject: QGraphicsScene: Fix UB (invalid cast) in removeItemHelper() The variable 'item' may or may not contain a QGraphicsObject pointer. Using static_cast on an 'item' that isn't, is UB. Found by UBSan (which failed to print a message, but the function names gave it away): [...] #6 #7 0x00002b18813bec05 in __ubsan::checkDynamicType(void*, void*, unsigned long) () from /opt/gcc/trunk/lib64/libubsan.so.0 #8 0x00002b18813be0c3 in HandleDynamicTypeCacheMiss(__ubsan::DynamicTypeCacheMissData*, unsigned long, unsigned long, __ubsan::ReportOptions) () from /opt/gcc/trunk/lib64/libubsan.so.0 #9 0x00002b18813be783 in __ubsan_handle_dynamic_type_cache_miss () from /opt/gcc/trunk/lib64/libubsan.so.0 #10 0x00002b1875e71d4d in QGraphicsScenePrivate::removeItemHelper(QGraphicsItem*) () at /home/marc/Qt/qt5/qtbase/src/widgets/graphicsview/qgraphicsscene.cpp:720 #11 0x00002b1875e731ef in QGraphicsScene::removeItem(QGraphicsItem*) () at /home/marc/Qt/qt5/qtbase/src/widgets/graphicsview/qgraphicsscene.cpp:2929 #12 0x00002b1875e6d05f in QGraphicsScenePrivate::removeItemHelper(QGraphicsItem*) () at /home/marc/Qt/qt5/qtbase/src/widgets/graphicsview/qgraphicsscene.cpp:604 #13 0x00002b1875e731ef in QGraphicsScene::removeItem(QGraphicsItem*) () at /home/marc/Qt/qt5/qtbase/src/widgets/graphicsview/qgraphicsscene.cpp:2929 #14 0x00002b1875e73e68 in QGraphicsScene::addItem(QGraphicsItem*) () at /home/marc/Qt/qt5/qtbase/src/widgets/graphicsview/qgraphicsscene.cpp:2505 #15 0x000000000043d34d in tst_QGraphicsWidget::fontPropagationSceneChange() () at /home/marc/Qt/qt5/qtbase/tests/auto/widgets/graphicsview/qgraphicswidget/tst_qgraphicswidget.cpp:941 [...] Fix by using QGraphicsItem::toGraphicsObject(). Yes, it's that simple... Change-Id: If04d1b62603cfd808cc7b64946da536c221a0c11 Reviewed-by: Friedemann Kleint --- src/widgets/graphicsview/qgraphicsscene.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/widgets/graphicsview') diff --git a/src/widgets/graphicsview/qgraphicsscene.cpp b/src/widgets/graphicsview/qgraphicsscene.cpp index ad58aeb488..9e0be0c280 100644 --- a/src/widgets/graphicsview/qgraphicsscene.cpp +++ b/src/widgets/graphicsview/qgraphicsscene.cpp @@ -710,7 +710,7 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item) ++it; } - QGraphicsObject *dummy = static_cast(item); + QGraphicsObject *dummy = item->toGraphicsObject(); cachedTargetItems.removeOne(dummy); cachedItemGestures.remove(dummy); cachedAlreadyDeliveredGestures.remove(dummy); -- cgit v1.2.3 From 622681eb508ddb1bd51a39b6887beddb43218504 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Wed, 21 Sep 2016 10:01:01 +0200 Subject: QGraphicsScene: Fix UB (invalid cast) in Private::ungrabMouse() Found by UBSan: qgraphicsscene.cpp:1000:40: runtime error: downcast of address 0x2af0d4072b00 which does not point to an object of type 'QGraphicsWidget' 0x2af0d4072b00: note: object is of type 'QGraphicsObject' 00 00 00 00 30 f5 26 bd f0 2a 00 00 90 e1 05 d4 f0 2a 00 00 a8 e3 26 bd f0 2a 00 00 d0 33 0f d4 ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'QGraphicsObject' #0 0x2af0badf1305 in QGraphicsScenePrivate::ungrabMouse(QGraphicsItem*, bool) qgraphicsscene.cpp:1000 #1 0x2af0bae0fc24 in QGraphicsScenePrivate::removeItemHelper(QGraphicsItem*) qgraphicsscene.cpp:692 #2 0x2af0bacd21f6 in QGraphicsItem::~QGraphicsItem() qgraphicsitem.cpp:1555 #3 0x2af0bacd4c48 in QGraphicsObject::~QGraphicsObject() qgraphicsitem.cpp:7766 #4 0x2af0baf7e99c in QGraphicsWidget::~QGraphicsWidget() qgraphicswidget.cpp:231 #5 0x2af0baf7f8c0 in QGraphicsWidget::~QGraphicsWidget() qgraphicswidget.cpp:282 #6 0x2af0badcee34 in QGraphicsScene::clear() qgraphicsscene.cpp:2388 #7 0x2af0badcf3fc in QGraphicsScene::~QGraphicsScene() qgraphicsscene.cpp:1682 #8 0x4b26f0 in tst_QGraphicsWidget::popupMouseGrabber() tst_qgraphicswidget.cpp:47 Fix by using the existing graphics widget pointer, determined a line above to be equivalent to 'item', for the removePopup() function call instead of casting 'item' itself. The rest of removePopup() appears to be well-behaved and doesn't trigger any more UBSan errors, so it was indeed just the cast which was undefined, no member calls. Change-Id: Ia54da90262a7a02f527914a90b0208be0ffc0f0b Reviewed-by: Thiago Macieira --- src/widgets/graphicsview/qgraphicsscene.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/widgets/graphicsview') diff --git a/src/widgets/graphicsview/qgraphicsscene.cpp b/src/widgets/graphicsview/qgraphicsscene.cpp index 9e0be0c280..ebc521eb00 100644 --- a/src/widgets/graphicsview/qgraphicsscene.cpp +++ b/src/widgets/graphicsview/qgraphicsscene.cpp @@ -987,7 +987,7 @@ void QGraphicsScenePrivate::ungrabMouse(QGraphicsItem *item, bool itemIsDying) // If the item is a popup, go via removePopup to ensure state // consistency and that it gets hidden correctly - beware that // removePopup() reenters this function to continue removing the grab. - removePopup((QGraphicsWidget *)item, itemIsDying); + removePopup(popupWidgets.constLast(), itemIsDying); return; } -- cgit v1.2.3 From 8e45fe6d6c2084752983d905cf22f777e7062baf Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Wed, 21 Sep 2016 09:12:24 +0200 Subject: QGraphicsScene: don't search for nullptr Following f6cb8b1af8f15a06898c5c71f81c64779d9478f6, take advantage of the nullptr return case of QGraphicsItem ::toGraphicsObject() by not looking up nullptr in the QList and the two QHashes. They don't contain nullptrs. Change-Id: Ic1cfbb4c60061577a09348ef78fdc573f95ad9a8 Reviewed-by: Friedemann Kleint --- src/widgets/graphicsview/qgraphicsscene.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'src/widgets/graphicsview') diff --git a/src/widgets/graphicsview/qgraphicsscene.cpp b/src/widgets/graphicsview/qgraphicsscene.cpp index ebc521eb00..5e3b426d49 100644 --- a/src/widgets/graphicsview/qgraphicsscene.cpp +++ b/src/widgets/graphicsview/qgraphicsscene.cpp @@ -710,10 +710,11 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item) ++it; } - QGraphicsObject *dummy = item->toGraphicsObject(); - cachedTargetItems.removeOne(dummy); - cachedItemGestures.remove(dummy); - cachedAlreadyDeliveredGestures.remove(dummy); + if (QGraphicsObject *dummy = item->toGraphicsObject()) { + cachedTargetItems.removeOne(dummy); + cachedItemGestures.remove(dummy); + cachedAlreadyDeliveredGestures.remove(dummy); + } foreach (Qt::GestureType gesture, item->d_ptr->gestureContext.keys()) ungrabGesture(item, gesture); -- cgit v1.2.3 From b9e42067268bc80d126e82c4d892ffe33bb4c17a Mon Sep 17 00:00:00 2001 From: Palo Kisa Date: Mon, 26 Sep 2016 22:16:55 +0200 Subject: QGraphicsAnchorLayout: Fix invalid use of Q_AUTOTEST_EXPORT The Q_AUTOTEST_EXPORT is defined in all cases. So usage as #if defined(Q_AUTOTEST_EXPORT) was wrong. Change-Id: Ia1c1526ad08fdfa35ca773d7c62f8bbba39a6d38 Reviewed-by: Marc Mutz Reviewed-by: Oswald Buddenhagen --- src/widgets/graphicsview/qgraphicsanchorlayout_p.cpp | 4 ++-- src/widgets/graphicsview/qgraphicsanchorlayout_p.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'src/widgets/graphicsview') diff --git a/src/widgets/graphicsview/qgraphicsanchorlayout_p.cpp b/src/widgets/graphicsview/qgraphicsanchorlayout_p.cpp index dac8e61645..8c27af1da5 100644 --- a/src/widgets/graphicsview/qgraphicsanchorlayout_p.cpp +++ b/src/widgets/graphicsview/qgraphicsanchorlayout_p.cpp @@ -2090,7 +2090,7 @@ QList getVariables(const QList &constraints) void QGraphicsAnchorLayoutPrivate::calculateGraphs( QGraphicsAnchorLayoutPrivate::Orientation orientation) { -#if defined(QT_DEBUG) || defined(Q_AUTOTEST_EXPORT) +#if defined(QT_DEBUG) || defined(QT_BUILD_INTERNAL) lastCalculationUsedSimplex[orientation] = false; #endif @@ -2254,7 +2254,7 @@ bool QGraphicsAnchorLayoutPrivate::calculateTrunk(Orientation orientation, const sizeHints[orientation][Qt::MaximumSize] = ad->sizeAtMaximum; } -#if defined(QT_DEBUG) || defined(Q_AUTOTEST_EXPORT) +#if defined(QT_DEBUG) || defined(QT_BUILD_INTERNAL) lastCalculationUsedSimplex[orientation] = needsSimplex; #endif diff --git a/src/widgets/graphicsview/qgraphicsanchorlayout_p.h b/src/widgets/graphicsview/qgraphicsanchorlayout_p.h index a5c7f1e2ce..4f8a106811 100644 --- a/src/widgets/graphicsview/qgraphicsanchorlayout_p.h +++ b/src/widgets/graphicsview/qgraphicsanchorlayout_p.h @@ -568,7 +568,7 @@ public: bool graphHasConflicts[2]; QSet m_floatItems[2]; -#if defined(QT_DEBUG) || defined(Q_AUTOTEST_EXPORT) +#if defined(QT_DEBUG) || defined(QT_BUILD_INTERNAL) bool lastCalculationUsedSimplex[2]; #endif -- cgit v1.2.3