diff options
author | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2022-07-06 02:20:38 +0200 |
---|---|---|
committer | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2022-07-10 23:40:24 +0200 |
commit | 6f852935e0c847f91c7d6ffca455a74307ebe26d (patch) | |
tree | 037cd5c792abf48ef86535f7a249bffef84e5913 /tests | |
parent | 54b276be0b5885bbaee2c38f472eb39731fd684a (diff) |
QString: fix lifetime issues with QRegularExpression APIs
QString has several functions taking a QRegularExpression: indexOf(),
contains(), and so on. Some of those have an out-argument of type
QRegularExpressionMatch, to report the details of the match (if any).
For instance:
QRegularExpression re(...);
QRegularExpressionMatch match;
if (string.contains(re, &match))
use(match);
The code used to route the implementation of these functions through
QStringView (which has the very same functions). This however opens
up a lifetime problem with temporary strings:
if (getString().contains(re, &match))
use(match); // match is dangling
Here `match` is dangling because it is referencing data into the
destroyed temporary -- nothing is keeping the string alive. This is
against the rules we've decided for Qt, and it's also asymmetric with
the corresponding code that uses QRegularExpression directly instead:
match = re.match(getString());
if (match.hasMatch())
use(match); // not dangling
... although we've documented not to do this. (In light of the decision
we've made w.r.t. temporaries, the documentation is wrong anyways.)
Here QRE takes a copy of the string and stores it in the match object,
thus keeping it alive.
Hence, extend the implementation of the QString functions to keep a
(shallow) copy of the string. To keep the code shared as much as
possible with QStringView, in theory one could have a function taking a
std::variant<QString, QStringView> and that uses the currently active
member. However I've found that std::variant here creates some abysmal
codegen, so instead I went for a simpler approach -- pass a QStringView
and an optional pointer to a QString. Use the latter if it's loaded.
QStringView has some inline code that calls into exported functions, so
I can't change the signature of them without breaking BC; I'm instead
adding new unexported functions and a Qt 7 note to unify them.
Change-Id: I7c65885a84069d0fbb902dcc96ddff543ca84562
Fixes: QTBUG-103940
Pick-to: 6.2 6.3 6.4
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Diffstat (limited to 'tests')
-rw-r--r-- | tests/auto/corelib/text/qstring/tst_qstring.cpp | 40 |
1 files changed, 40 insertions, 0 deletions
diff --git a/tests/auto/corelib/text/qstring/tst_qstring.cpp b/tests/auto/corelib/text/qstring/tst_qstring.cpp index 467d5b0b39..239b3498a9 100644 --- a/tests/auto/corelib/text/qstring/tst_qstring.cpp +++ b/tests/auto/corelib/text/qstring/tst_qstring.cpp @@ -537,6 +537,7 @@ private slots: #if QT_CONFIG(regularexpression) void split_regularexpression_data(); void split_regularexpression(); + void regularexpression_lifetime(); #endif void fromUtf16_data(); void fromUtf16(); @@ -6244,6 +6245,45 @@ void tst_QString::split_regularexpression() split_regexp<QStringList, QRegularExpression>(string, pattern, result); split_regexp<QList<QStringView>, QRegularExpression>(string, pattern, result); } + +// Test that rvalue strings (e.g. temporaries) are kept alive in +// QRegularExpression-related APIs +void tst_QString::regularexpression_lifetime() +{ + const auto getString = [] { + // deliberately heap-allocated + return QString(QLatin1String("the quick brown fox jumps over the lazy dog")); + }; + + QRegularExpression re("\\w{5}"); + + { + QString s = getString(); + QRegularExpressionMatch match; + const bool contains = std::move(s).contains(re, &match); + s.fill('X'); // NOLINT(bugprone-use-after-move) + QVERIFY(contains); + QCOMPARE(match.capturedView(), u"quick"); + } + + { + QString s = getString(); + QRegularExpressionMatch match; + const auto index = std::move(s).indexOf(re, 0, &match); + s.fill('X'); // NOLINT(bugprone-use-after-move) + QCOMPARE(index, 4); + QCOMPARE(match.capturedView(), u"quick"); + } + + { + QString s = getString(); + QRegularExpressionMatch match; + const auto lastIndex = std::move(s).lastIndexOf(re, &match); + s.fill('X'); // NOLINT(bugprone-use-after-move) + QCOMPARE(lastIndex, 20); + QCOMPARE(match.capturedView(), u"jumps"); + } +} #endif void tst_QString::fromUtf16_data() |