[Development] QArrayDataObs/Pointer UB post-mortem

[Marc Mutz]

Hi,

No TL;DR: You're supposed to read and understand all of it.

As I write this, the UB fix for QArrayDataObs/Pointer¹ that held up² adding RHEL10 to the CI has merged to all branches, except the ESM ones (6.5 and 5.15), but it's just a keystroke of Jani's away there, too.

If the following reads like "I told you so", I'm sorry, but ... I did ;)

Act 1: Phantom Menace

When we were trying to enable RHEL 10 (using GCC 14.3) in the CI, we hit a FAIL in tst_qarraydata. This is _good_. It means it didn't blow up at some customer first (for all we know). Several highly-esteemed developers and two chat bots concluded that this is a compiler bug. The problem is that the compiler isn't very exotic, so what does it bring us if we know it's a compiler bug? Nothing. We'd still need to work around it.

Act 2: Deus Ex Machina

It just so happens that this developer ran into a very similar issue last year, and found, with the help of the GCC devs³, that the compiler is completely correct, and it was my fault. I was casting a char*& to a reference to a pointer to enum qchar8 : uchar {}, incremented it, and expected the original pointer object to have been incremented. But this is not guaranteed by the standard. Only char etc is allowed to alias other objects, _not_ char*, and certainly not enum*. So the compiler was right in caching the old value of the object and continue using it.

Side note: if this sounds familiar to you, it's because we've just talked about that this is exactly how the UB in QObject::disconnect() would likely manifest.⁴

The analysis of the QArrayData assembly sounded eerily similar: one of the three fields that QArrayDataPointer::swap() swaps wasn't swapped. Thiago found the culprit⁵, I merely connected the dots: we were inheriting QArrayDataOps : QArrayDataPointer, and static_cast<Ops*>(this) from QArrayDataPointer::op->(). The cast is UB, because *this isn't actually an object of type Ops, but only QArrayDataPointer. Period. This code has been stitting there for 15 years (since Qt 5.0, but unused until Qt 6.0)⁵, and not making any problems. Until an innocent compiler upgrade.

Side note: Casting a Base to a Derived : Base is UB if the object isn't actually of dynamic type Derived. It doesn't matter whether sizeof(Derived) == sizeof(Base). It _is_ always UB. I have lost count of the number of code snippets I have fixed in tests, mostly, where ubsan complained about invalid casts where it turned out that this "technique" was used. It was _very_ prevalent in Qt, as if somone somewhere put up the rule that, regardless of what the std _actually_ says, casting to a non-actual derived type is ok as long as the sizeof's are the same. Well, it's not.

Once the bug was identified, the fix was lengthy, but pretty straight-forward. But since this was "actively exploited" UB in one of Qt's most central classes (QList, QString, QByteArray depend on this, so I'd be hard-pressed to find anything in Qt that does _not_ depend on QArrayData), we decided that we need to pick this all the way back, incl. into ESM branches.

Act 3: Odyssey

This is where the vast amount of my work was sunk: cherry-picking through a forest of cleanup and rewrite commits.

I have long said that, from a software-engineering POV, you do _not_ want your active branches to diverge very much. If you find a security issue, speed is of the essence, and you a) don't want to slow down by resolving conflicts (you want to stage the change in dev and wake up the next morning with the change merged to all branches by the bots) and b) don't want to introduce a lot of risk by combining commit sets in a unique combination found nowhere else.

Well, this bug is as close to a security issue as you can have without an actual security issue. And it ran full steam into one the worst violations of that principle, which, I hasten to add, is _not_ standing Qt policy.

The end effect is that, the issue took a full week to make it to the ESM branches, instead of a few hours. It didn't help that the cherry-picking has its own weird ideas about where to pick to⁶, but that only manifests in multi-commit fixing scenarios, so doesn't affect your typical security fix (which tend to be one-liners more often than not).

Epilogue

To me, this issue reinforces what I have known to be true for years, to wit:
- the compiler is more clever than you, you MUST NOT fall into UB, regardless of whether _you_ think it's benign (even if you think you can prove it)
- only features shouldn't be picked back, in particular
  - test changes should always be picked back, with a XFAIL, if necessary
  - refactorings should always be picked back (or not done at all)
  - bugfixes should always be picked back

We have known issues in both categories. I'm pretty sure we have more classes in Qt that are never instantiated, only cast to, and we have at least one module (you know who you are), which is security-critical, and where a high code turnover is paired with lack of cherry-picking. I have experienced the same frustrating conflicts there than I did in shoveling QArrayData patches though. That's not to single out said module or it's authors and maintainers. They work by the book. It's the book that's wrong here, and I would hope we have a discussion about changing it to aggressive picking going forward, for security-critical components. We have the CRA markings now. I would say that one goal should be that all security-critical components in Qt are at the same level in all active branches. This is what we do with 3rd-party components already. It seems odd to exclude Qt's own security-critical components, where we have the added benefit of being able to exclude feature commits from picking. In 3rd-party components, we always get both.

Hope this provides some food for thought (and change).

Thanks for reading this far,
Marc

¹ https://codereview.qt-project.org/c/qt/qtbase/+/736274 / https://codereview.qt-project.org/q/topic:QArrayDataOps-2026
² https://qt-project.atlassian.net/browse/QTBUG-146652 / https://qt-project.atlassian.net/browse/QTQAINFRA-7202
³ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=121593
⁴ https://lists.qt-project.org/pipermail/development/2026-March/047026.html
⁵ introduced, but not necessarily taken into use, by https://codereview.qt-project.org/c/qt/qtbase/+/9847
⁶ https://qt-project.atlassian.net/browse/QTQAINFRA-7900 / https://qt-project.atlassian.net/browse/QTQAINFRA-7903


--
Marc Mutz <marc.mutz at qt.io><mailto:marc.mutz at qt.io> (he/his)
Principal Software Engineer

The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io<http://www.qt.io>

Geschäftsführer: Mika Pälsi, Juha Varelius, Juha Puputti
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B

Public
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20260526/ee60e762/attachment-0001.htm>