From 05b91b119523a8cc4f8134bd997b35ac02b73ddf Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Tue, 4 Feb 2025 12:09:12 +0100 Subject: [PATCH] Fix UB in QTextStreamPrivate::putNumber() Negating a negative value may attempt to negate the minimal value for the signed type, whose only set bit is its sign; this is UB. So don't do that. Since the non-decimal branch of the code just prepends locale.negativeSign(), and we're passing -1 as width parameter to the QLocalePrivate formatters (so there's no zero-padding, whose size would be reduced to make space for a sign), we can treat decimal the same as all other bases. This, furthermore, simplifies the code. In the process, I noticed (because a test only done for decimal failed) that if QTextStream::ForceSign is set, this code-path for negatives would prepend the minus sign before a plus sign supplied by QLocaleData. Skip the flag to include sign, for negative input, since we'll be including a negative sign anyway. Remove the QEXPECT_FAIL() from the test I've added for this in a preparatory commit. Purge one now-redundant comment and revise some others. Expand on why we need to hack octal zero to display two zeros. Add a second test row to keep the lonely zero-row company in the test for that. Fixes: QTBUG-133269 Pick-to: 6.8 6.5 5.15 Change-Id: I35c9bdf34b812cff578de9b0a2570a60e6145c80 Reviewed-by: Marc Mutz Reviewed-by: Thiago Macieira (cherry picked from commit 44ddb34da07f7fdb2214cb39b56cf114d4aa209e) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/serialization/qtextstream.cpp | 34 +++++++------------ .../qtextstream/tst_qtextstream.cpp | 2 +- 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/corelib/serialization/qtextstream.cpp b/src/corelib/serialization/qtextstream.cpp index 2b6aed5d21c..372091a9d7f 100644 --- a/src/corelib/serialization/qtextstream.cpp +++ b/src/corelib/serialization/qtextstream.cpp @@ -2204,44 +2204,34 @@ QTextStream &QTextStream::operator>>(char *c) */ void QTextStreamPrivate::putNumber(qulonglong number, bool negative) { - QString result; - unsigned flags = 0; const QTextStream::NumberFlags numberFlags = params.numberFlags; if (numberFlags & QTextStream::ShowBase) flags |= QLocaleData::ShowBase; - if (numberFlags & QTextStream::ForceSign) + // ForceSign is irrelevant when we'll be including a sign in any case: + if ((numberFlags & QTextStream::ForceSign) && !negative) flags |= QLocaleData::AlwaysShowSign; if (numberFlags & QTextStream::UppercaseBase) flags |= QLocaleData::UppercaseBase; if (numberFlags & QTextStream::UppercaseDigits) flags |= QLocaleData::CapitalEorX; - // add thousands group separators. For backward compatibility we - // don't add a group separator for C locale. + // Group digits. For backward compatibility, we skip this for the C locale. if (locale != QLocale::c() && !locale.numberOptions().testFlag(QLocale::OmitGroupSeparator)) flags |= QLocaleData::GroupDigits; const QLocaleData *dd = locale.d->m_data; int base = params.integerBase ? params.integerBase : 10; - if (negative && base == 10) { - result = dd->longLongToString(-static_cast(number), -1, - base, -1, flags); - } else if (negative) { - // Workaround for backward compatibility for writing negative - // numbers in octal and hex: - // QTextStream(result) << Qt::showbase << Qt::hex << -1 << oct << -1 - // should output: -0x1 -0b1 - result = dd->unsLongLongToString(number, -1, base, -1, flags); + QString result = dd->unsLongLongToString(number, -1, base, -1, flags); + if (negative) { result.prepend(locale.negativeSign()); - } else { - result = dd->unsLongLongToString(number, -1, base, -1, flags); - // workaround for backward compatibility - in octal form with - // ShowBase flag set zero should be written as '00' - if (number == 0 && base == 8 && params.numberFlags & QTextStream::ShowBase - && result == "0"_L1) { - result.prepend(u'0'); - } + } else if (number == 0 && base == 8 && params.numberFlags & QTextStream::ShowBase + && result == "0"_L1) { + // Workaround for backward compatibility - in octal form with ShowBase + // flag set, zero should get its 0 prefix before its 0 value, but + // QLocalePrivate only adds the prefix if the number doesn't start with + // a zero. + result.prepend(u'0'); } putString(result, true); } diff --git a/tests/auto/corelib/serialization/qtextstream/tst_qtextstream.cpp b/tests/auto/corelib/serialization/qtextstream/tst_qtextstream.cpp index d66cc21e318..37aeb3f1d47 100644 --- a/tests/auto/corelib/serialization/qtextstream/tst_qtextstream.cpp +++ b/tests/auto/corelib/serialization/qtextstream/tst_qtextstream.cpp @@ -1123,6 +1123,7 @@ void tst_QTextStream::octTest_data() QTest::addColumn("data"); QTest::newRow("0") << 0 << QByteArray("00"); + QTest::newRow("40") << 40 << QByteArray("050"); } // ------------------------------------------------------------------------------ @@ -2716,7 +2717,6 @@ void tst_QTextStream::manipulators() stream << textData; stream.flush(); - QEXPECT_FAIL("hex-negative", "Discovered while fixing QTBUG-133269", Continue); QCOMPARE(buffer.data(), result); }