From 2e9d33e5344c75d0b91307aebe2bfa2fded1268f Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Wed, 19 Jul 2023 15:14:31 +0200 Subject: [PATCH] Use correct index for QLocale::system()'s static Nothing prevents client code from calling QLocale::setDefault() before we ever instantiate QLocale::system() - aside from some quirks that mean setDefault(), currently, does instantiate QLocale::system() to force initialization of defaultLocalePrivate - so using defaultIndex() could set the system QLocalePrivate instance's index incorrectly. In any case, even if the index is initially set correctly, a subsequent change to the system locale would change the correct index; and nothing outside QLocale::system() has access to the instance that would then be remembering an out-of-date index. Actually tripping over that inconsistency took some deviousness, but was possible. The index is (currently) only used for month name lookups and those special-case, for the Roman-derived calendars, the system locale, to only use the index if the system locale offers no name for a month. Meanwhile, updateSystemPrivate() uses the fallback locale's index for its look-up of which CLDR data to copy into the fallback QLocaleData for the system locale. None the less, a non-Roman calendar's lookup will go via the index to get at the CLDR data for that calendar, thereby exposing the system locale's index to use; and, sure enough, a setDefault() could lead that to produce wrong answers. In QLocale::system() there's a cached QLocalePrivate, whose index we need to ensure stays in sync with the active system locale. So pass its &m_index to systemData(), which will now (when passed this) ensure it's up to date. Since we always have called systemData(), to ensure it is up to date, we can skip that update in the initialization of the cached private and use m_index = -1 to let systemData() know when it's in the initial call, thereby making the static cache constinit. Amended a test to what proved the issue was present. Change-Id: I8d7ab5830cf0bbb9265c2af2a1edc9396ddef79f Reviewed-by: Thiago Macieira Reviewed-by: Qt CI Bot --- src/corelib/text/qlocale.cpp | 40 ++++++++++-- src/corelib/text/qlocale_p.h | 2 +- .../qlocale/syslocaleapp/syslocaleapp.cpp | 10 ++- .../auto/corelib/text/qlocale/tst_qlocale.cpp | 64 ++++++++++--------- 4 files changed, 77 insertions(+), 39 deletions(-) diff --git a/src/corelib/text/qlocale.cpp b/src/corelib/text/qlocale.cpp index d15bdd93cd..c7ecddb484 100644 --- a/src/corelib/text/qlocale.cpp +++ b/src/corelib/text/qlocale.cpp @@ -788,25 +788,42 @@ static void updateSystemPrivate() } #endif // !QT_NO_SYSTEMLOCALE -static const QLocaleData *systemData() +static const QLocaleData *systemData(qsizetype *sysIndex = nullptr) { #ifndef QT_NO_SYSTEMLOCALE /* Copy over the information from the fallback locale and modify. - This modifies (cross-thread) global state, so take care to only call it in - one thread. + If sysIndex is passed, it should be the m_index of the system locale's + QLocalePrivate, which we'll update if it needs it. + + This modifies (cross-thread) global state, so is mutex-protected. */ { + Q_CONSTINIT static QLocaleId sysId; + bool updated = false; + Q_CONSTINIT static QBasicMutex systemDataMutex; systemDataMutex.lock(); - if (systemLocaleData.m_language_id == 0) + if (systemLocaleData.m_language_id == 0) { updateSystemPrivate(); + updated = true; + } + // Initialization of system private has *sysIndex == -1 to hit this. + if (sysIndex && (updated || *sysIndex < 0)) { + const QLocaleId nowId = systemLocaleData.id(); + if (sysId != nowId || *sysIndex < 0) { + // This look-up may be expensive: + *sysIndex = QLocaleData::findLocaleIndex(nowId); + sysId = nowId; + } + } systemDataMutex.unlock(); } return &systemLocaleData; #else + Q_UNUSED(sysIndex); return locale_data; #endif } @@ -2749,8 +2766,19 @@ QString QLocale::toString(double f, char format, int precision) const QLocale QLocale::system() { - QT_PREPEND_NAMESPACE(systemData)(); // Ensure system data is up to date. - static QLocalePrivate locale(systemData(), defaultIndex(), DefaultNumberOptions, 1); + constexpr auto sysData = []() { + // Same return as systemData(), but leave the setup to the actual call to it. +#ifdef QT_NO_SYSTEMLOCALE + return locale_data; +#else + return &systemLocaleData; +#endif + }; + Q_CONSTINIT static QLocalePrivate locale(sysData(), -1, DefaultNumberOptions, 1); + // Calling systemData() ensures system data is up to date; we also need it + // to ensure that locale's index stays up to date: + systemData(&locale.m_index); + Q_ASSERT(locale.m_index >= 0 && locale.m_index < locale_data_size); return QLocale(locale); } diff --git a/src/corelib/text/qlocale_p.h b/src/corelib/text/qlocale_p.h index 6c1826c846..4dcfb0329b 100644 --- a/src/corelib/text/qlocale_p.h +++ b/src/corelib/text/qlocale_p.h @@ -532,7 +532,7 @@ public: // System locale has an m_data all its own; all others have m_data = locale_data + m_index const QLocaleData *const m_data; QBasicAtomicInt ref; - const qsizetype m_index; + qsizetype m_index; // System locale needs this updated when m_data->id() changes. QLocale::NumberOptions m_numberOptions; static QBasicAtomicInt s_generation; diff --git a/tests/auto/corelib/text/qlocale/syslocaleapp/syslocaleapp.cpp b/tests/auto/corelib/text/qlocale/syslocaleapp/syslocaleapp.cpp index 1194cf0895..7cfdf161ea 100644 --- a/tests/auto/corelib/text/qlocale/syslocaleapp/syslocaleapp.cpp +++ b/tests/auto/corelib/text/qlocale/syslocaleapp/syslocaleapp.cpp @@ -1,15 +1,21 @@ // Copyright (C) 2016 The Qt Company Ltd. // SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0 #include +#include #include #include int main(int argc, char** argv) { QCoreApplication app(argc, argv); - QLocale l; + // Setting a default locale should not mess up the system one. + QLocale::setDefault(QLocale::Persian); + QLocale l = QLocale::system(); + // A non-Roman calendar will use CLDR data instead of system data, so needs + // to have got the right locale index to look that up. + QCalendar cal = QCalendar(QCalendar::System::Jalali); QTextStream str(stdout); - str << l.name(); + str << l.name() << ' ' << cal.standaloneMonthName(l, 2); return 0; } diff --git a/tests/auto/corelib/text/qlocale/tst_qlocale.cpp b/tests/auto/corelib/text/qlocale/tst_qlocale.cpp index 912acfd42d..65d7d9ecbf 100644 --- a/tests/auto/corelib/text/qlocale/tst_qlocale.cpp +++ b/tests/auto/corelib/text/qlocale/tst_qlocale.cpp @@ -591,45 +591,49 @@ void tst_QLocale::systemLocale_data() // Note that the accepted values for fields are implementation-dependent; // the template is language[_territory][.codeset][@modifier] + // "Ordibehesht" is the name (as adapted to English, German or Norsk) of the + // second month of the Jalali calendar. If you see anything in Arabic, + // setDefault(Persian) has interfered with the system locale setup. + // Vanilla: - ADD_CTOR_TEST("C", "C"); + ADD_CTOR_TEST("C", "C Ordibehesht"); // Standard forms: - ADD_CTOR_TEST("en", "en_US"); - ADD_CTOR_TEST("en_GB", "en_GB"); - ADD_CTOR_TEST("de", "de_DE"); + ADD_CTOR_TEST("en", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_GB", "en_GB Ordibehesht"); + ADD_CTOR_TEST("de", "de_DE Ordibehescht"); // Norsk has some quirks: - ADD_CTOR_TEST("no", "nb_NO"); - ADD_CTOR_TEST("nb", "nb_NO"); - ADD_CTOR_TEST("nn", "nn_NO"); - ADD_CTOR_TEST("no_NO", "nb_NO"); - ADD_CTOR_TEST("nb_NO", "nb_NO"); - ADD_CTOR_TEST("nn_NO", "nn_NO"); + ADD_CTOR_TEST("no", "nb_NO ordibehesht"); + ADD_CTOR_TEST("nb", "nb_NO ordibehesht"); + ADD_CTOR_TEST("nn", "nn_NO ordibehesht"); + ADD_CTOR_TEST("no_NO", "nb_NO ordibehesht"); + ADD_CTOR_TEST("nb_NO", "nb_NO ordibehesht"); + ADD_CTOR_TEST("nn_NO", "nn_NO ordibehesht"); // Not too fussy about case: - ADD_CTOR_TEST("DE", "de_DE"); - ADD_CTOR_TEST("EN", "en_US"); + ADD_CTOR_TEST("DE", "de_DE Ordibehescht"); + ADD_CTOR_TEST("EN", "en_US Ordibehesht"); // Invalid fields - ADD_CTOR_TEST("bla", "C"); - ADD_CTOR_TEST("zz", "C"); - ADD_CTOR_TEST("zz_zz", "C"); - ADD_CTOR_TEST("zz...", "C"); - ADD_CTOR_TEST("en.bla", "en_US"); - ADD_CTOR_TEST("en@bla", "en_US"); - ADD_CTOR_TEST("en_blaaa", "en_US"); - ADD_CTOR_TEST("en_zz", "en_US"); - ADD_CTOR_TEST("en_GB.bla", "en_GB"); - ADD_CTOR_TEST("en_GB@.bla", "en_GB"); - ADD_CTOR_TEST("en_GB@bla", "en_GB"); + ADD_CTOR_TEST("bla", "C Ordibehesht"); + ADD_CTOR_TEST("zz", "C Ordibehesht"); + ADD_CTOR_TEST("zz_zz", "C Ordibehesht"); + ADD_CTOR_TEST("zz...", "C Ordibehesht"); + ADD_CTOR_TEST("en.bla", "en_US Ordibehesht"); + ADD_CTOR_TEST("en@bla", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_blaaa", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_zz", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_GB.bla", "en_GB Ordibehesht"); + ADD_CTOR_TEST("en_GB@.bla", "en_GB Ordibehesht"); + ADD_CTOR_TEST("en_GB@bla", "en_GB Ordibehesht"); // Empty optional fields, but with punctuators supplied - ADD_CTOR_TEST("en.", "en_US"); - ADD_CTOR_TEST("en@", "en_US"); - ADD_CTOR_TEST("en.@", "en_US"); - ADD_CTOR_TEST("en_", "en_US"); - ADD_CTOR_TEST("en_.", "en_US"); - ADD_CTOR_TEST("en_.@", "en_US"); + ADD_CTOR_TEST("en.", "en_US Ordibehesht"); + ADD_CTOR_TEST("en@", "en_US Ordibehesht"); + ADD_CTOR_TEST("en.@", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_.", "en_US Ordibehesht"); + ADD_CTOR_TEST("en_.@", "en_US Ordibehesht"); #undef ADD_CTOR_TEST #if QT_CONFIG(process) // for runSysApp @@ -638,7 +642,7 @@ void tst_QLocale::systemLocale_data() QString errorMessage; if (runSysApp(m_sysapp, QStringList(), cleanEnv, &defaultLoc, &errorMessage)) { #if defined(Q_OS_MACOS) - QString localeForInvalidLocale = "C"; + QString localeForInvalidLocale = "C Ordibehesht"; #else QString localeForInvalidLocale = defaultLoc; #endif