From 68e507a522ff338aaadbc2d891e6ad436843a9a7 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Tue, 25 Aug 2026 13:23:05 +1200 Subject: [PATCH] Reject non-finite element/terminal coordinates on load (fixes #781, #782) QString::toDouble() reports a successful conversion for "nan"/"inf"/ "-inf" -- confirmed directly -- so the existing conv_ok checks in Element::valideXml() and Terminal::valideXml() never caught a non-finite x/y. A NaN-positioned element reaching the scene can hang QGraphicsScene::addItem() forever: an existing conductor's itemChange() runs a collision test (calculateTextItemPosition() -> QGraphicsItem::collidesWithPath()) whose underlying QPathClipper spins without terminating when fed a NaN-valued QPainterPath, since NaN breaks the ordering comparisons the clipping algorithm's termination depends on (#781). Short of that, a non-finite value that doesn't happen to trigger a collision test simply gets written straight back out on save with nothing to stop it (#782). Element::valideXml() and Terminal::valideXml() now also check qIsFinite() on the parsed x/y, rejecting the whole item the same way a missing attribute already does. DynamicElementTextItem::fromXml() has no such reject-the-item gate (void return, no caller check), so its x/y are clamped to 0 instead -- the same fallback the attribute lookup already uses when x/y is missing entirely. Verified against both original findings' exact repro steps: - #781: Habitat-Unifilaire.qet with x="nan" on one element -- hung (SIGTERM'd by a 25s timeout) on an unfixed build, resaves cleanly (exit 0) on this one. - #782: grafcet.qet with y="nan" on a dynamic_elmt_text -- the value passed straight through to the resaved file on an unfixed build; clamped to 0 on this one. The specific field is now stable (y="0" on two consecutive resaves) where it read "nan" both times before. (grafcet.qet has an unrelated, already-known, unmerged fix (PR #779) for element/terminal-order non-determinism, so a whole-file diff across resaves still differs for reasons unconnected to this change -- checked the specific once-NaN field in isolation instead.) - Also checked -inf on affuteuse_250h.qet: same rejection, same result. Full qet-dbcheck.py sweep of the unmutated example corpus (23 projects), 0 regressions. Co-Authored-By: Claude Opus 5 --- sources/qetgraphicsitem/dynamicelementtextitem.cpp | 13 +++++++++++-- sources/qetgraphicsitem/element.cpp | 14 ++++++++++---- sources/qetgraphicsitem/terminal.cpp | 13 +++++++++---- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/sources/qetgraphicsitem/dynamicelementtextitem.cpp b/sources/qetgraphicsitem/dynamicelementtextitem.cpp index a9af725ae..8cd32830a 100644 --- a/sources/qetgraphicsitem/dynamicelementtextitem.cpp +++ b/sources/qetgraphicsitem/dynamicelementtextitem.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include /** @@ -221,8 +222,16 @@ void DynamicElementTextItem::fromXml(const QDomElement &dom_elmt) //Force the update of the displayed text setTextFrom(m_text_from); - QGraphicsTextItem::setPos(dom_elmt.attribute("x", QString::number(0)).toDouble(), - dom_elmt.attribute("y", QString::number(0)).toDouble()); + //QString::toDouble() accepts "nan"/"inf"/"-inf" as a successful + //conversion, so a corrupted or hand-edited file can hand this a + //non-finite position -- fall back to 0 the same way a missing + //attribute already does, rather than letting it reach the scene + //(see the matching comment in Element::valideXml(), the fromXml() + //gate a plain element's position goes through; this text item has + //no such gate to reject the whole item at, so it clamps instead). + double x = dom_elmt.attribute("x", QString::number(0)).toDouble(); + double y = dom_elmt.attribute("y", QString::number(0)).toDouble(); + QGraphicsTextItem::setPos(qIsFinite(x) ? x : 0, qIsFinite(y) ? y : 0); } /** diff --git a/sources/qetgraphicsitem/element.cpp b/sources/qetgraphicsitem/element.cpp index 23c9eda42..14b3a8f4f 100644 --- a/sources/qetgraphicsitem/element.cpp +++ b/sources/qetgraphicsitem/element.cpp @@ -53,6 +53,7 @@ static const QString plcTerminalKeys[] = { #include #include +#include #include class ElementXmlRetroCompatibility @@ -699,11 +700,16 @@ bool Element::valideXml(QDomElement &e) } bool conv_ok; - e.attribute(QStringLiteral("x")).toDouble(&conv_ok); - if (!conv_ok) return(false); + //QString::toDouble() accepts "nan"/"inf"/"-inf" and reports a + //successful conversion for them, so conv_ok alone doesn't reject a + //non-finite coordinate. A NaN position reaching the scene can hang + //QGraphicsScene::addItem() forever inside Qt's own polygon-clipping + //code when an existing conductor's collision test runs against it. + double x = e.attribute(QStringLiteral("x")).toDouble(&conv_ok); + if (!conv_ok || !qIsFinite(x)) return(false); - e.attribute(QStringLiteral("y")).toDouble(&conv_ok); - if (!conv_ok) return(false); + double y = e.attribute(QStringLiteral("y")).toDouble(&conv_ok); + if (!conv_ok || !qIsFinite(y)) return(false); return(true); } diff --git a/sources/qetgraphicsitem/terminal.cpp b/sources/qetgraphicsitem/terminal.cpp index ec862b39c..aa2b3024f 100644 --- a/sources/qetgraphicsitem/terminal.cpp +++ b/sources/qetgraphicsitem/terminal.cpp @@ -25,6 +25,7 @@ #include "../qetgraphicsitem/element.h" #include "conductortextitem.h" +#include #include QColor Terminal::neutralColor = QColor(Qt::blue); @@ -741,13 +742,17 @@ bool Terminal::valideXml(QDomElement &terminal) if (!terminal.hasAttribute("orientation")) return(false); bool conv_ok; + //QString::toDouble() accepts "nan"/"inf"/"-inf" and reports a + //successful conversion for them, so conv_ok alone doesn't reject a + //non-finite coordinate -- see the matching comment in + //Element::valideXml(). // parse l'abscisse - terminal.attribute("x").toDouble(&conv_ok); - if (!conv_ok) return(false); + double x = terminal.attribute("x").toDouble(&conv_ok); + if (!conv_ok || !qIsFinite(x)) return(false); // parse l'ordonnee - terminal.attribute("y").toDouble(&conv_ok); - if (!conv_ok) return(false); + double y = terminal.attribute("y").toDouble(&conv_ok); + if (!conv_ok || !qIsFinite(y)) return(false); // parse l'id terminal.attribute("id").toInt(&conv_ok);