ICU-23348 Fix thread safety issues with parsing and formatting in RuleBasedNumberFormat

Author: grhoten

icu4c/source/i18n/rbnf.cpp

@@ -785,6 +785,10 @@ RuleBasedNumberFormat::operator=(const RuleBasedNumberFormat& rhs)
     setDefaultRuleSet(rhs.getDefaultRuleSetName(), status);
     setRoundingMode(rhs.getRoundingMode());
 
+    if (lenient) {
+        initializeCollator(status);
+    }
+
     capitalizationInfoSet = rhs.capitalizationInfoSet;
     capitalizationForUIListMenu = rhs.capitalizationForUIListMenu;
     capitalizationForStandAlone = rhs.capitalizationForStandAlone;
@@ -1202,7 +1206,11 @@ RuleBasedNumberFormat::adjustForCapitalizationContext(int32_t startPos,
                 (capitalizationContext == UDISPCTX_CAPITALIZATION_FOR_STANDALONE && capitalizationForStandAlone)) ) {
             // titlecase first word of currentResult, here use sentence iterator unlike current implementations
             // in LocaleDisplayNamesImpl::adjustForUsageAndContext and RelativeDateFormat::format
-            currentResult.toTitle(capitalizationBrkIter, locale, U_TITLECASE_NO_LOWERCASE | U_TITLECASE_NO_BREAK_ADJUSTMENT);
+            // Clone the break iterator to avoid mutating it in a const method (thread safety).
+            LocalPointer<BreakIterator> iter(capitalizationBrkIter->clone());
+            if (iter.isValid()) {
+                currentResult.toTitle(iter.getAlias(), locale, U_TITLECASE_NO_LOWERCASE | U_TITLECASE_NO_BREAK_ADJUSTMENT);
+            }
         }
     }
 #endif
@@ -1272,6 +1280,10 @@ RuleBasedNumberFormat::setLenient(UBool enabled)
     if (!enabled && collator) {
         delete collator;
         collator = nullptr;
+    } else if (enabled && collator == nullptr) {
+        // Eagerly initialize the collator so that getCollator() is thread-safe.
+        UErrorCode status = U_ZERO_ERROR;
+        initializeCollator(status);
     }
 #endif
 }
@@ -1689,55 +1701,52 @@ RuleBasedNumberFormat::dispose()
 //-----------------------------------------------------------------------
 
 /**
- * Returns the collator to use for lenient parsing.  The collator is lazily created:
- * this function creates it the first time it's called.
- * @return The collator to use for lenient parsing, or null if lenient parsing
- * is turned off.
-*/
-const RuleBasedCollator*
-RuleBasedNumberFormat::getCollator() const
+ * Initializes the collator for lenient parsing. Must be called from
+ * non-const methods (setLenient, operator=) so that getCollator() can
+ * be thread-safe.
+ */
+void
+RuleBasedNumberFormat::initializeCollator(UErrorCode &status)
 {
 #if !UCONFIG_NO_COLLATION
-    if (!fRuleSets) {
-        return nullptr;
+    if (collator != nullptr || !fRuleSets) {
+        return;
     }
 
-    // lazy-evaluate the collator
-    if (collator == nullptr && lenient) {
-        // create a default collator based on the formatter's locale,
-        // then pull out that collator's rules, append any additional
-        // rules specified in the description, and create a _new_
-        // collator based on the combination of those rules
-
-        UErrorCode status = U_ZERO_ERROR;
+    Collator* temp = Collator::createInstance(locale, status);
+    RuleBasedCollator* newCollator;
+    if (U_SUCCESS(status) && (newCollator = dynamic_cast<RuleBasedCollator*>(temp)) != nullptr) {
+        if (lenientParseRules) {
+            UnicodeString rules(newCollator->getRules());
+            rules.append(*lenientParseRules);
 
-        Collator* temp = Collator::createInstance(locale, status);
-        RuleBasedCollator* newCollator;
-        if (U_SUCCESS(status) && (newCollator = dynamic_cast<RuleBasedCollator*>(temp)) != nullptr) {
-            if (lenientParseRules) {
-                UnicodeString rules(newCollator->getRules());
-                rules.append(*lenientParseRules);
-
-                newCollator = new RuleBasedCollator(rules, status);
-                // Exit if newCollator could not be created.
-                if (newCollator == nullptr) {
-                    return nullptr;
-                }
-            } else {
-                temp = nullptr;
-            }
-            if (U_SUCCESS(status)) {
-                newCollator->setAttribute(UCOL_DECOMPOSITION_MODE, UCOL_ON, status);
-                // cast away const
-                const_cast<RuleBasedNumberFormat*>(this)->collator = newCollator;
-            } else {
-                delete newCollator;
+            newCollator = new RuleBasedCollator(rules, status);
+            // Exit if newCollator could not be created.
+            if (newCollator == nullptr) {
+                status = U_MEMORY_ALLOCATION_ERROR;
+                return;
             }
+        } else {
+            temp = nullptr;
+        }
+        if (U_SUCCESS(status)) {
+            newCollator->setAttribute(UCOL_DECOMPOSITION_MODE, UCOL_ON, status);
+            collator = newCollator;
+        } else {
+            delete newCollator;
         }
-        delete temp;
     }
+    delete temp;
 #endif
+}
 
+/**
+ * Returns the collator to use for lenient parsing, or null if lenient parsing
+ * is turned off.
+*/
+const RuleBasedCollator*
+RuleBasedNumberFormat::getCollator() const
+{
     // if lenient-parse mode is off, this will be null
     // (see setLenientParseMode())
     return collator;

icu4c/source/i18n/unicode/rbnf.h

@@ -1131,6 +1131,7 @@ class U_I18N_API_CLASS RuleBasedNumberFormat : public NumberFormat {
     friend class FractionalPartSubstitution;
 
     inline NFRuleSet * getDefaultRuleSet() const;
+    void initializeCollator(UErrorCode &status);
     const RuleBasedCollator * getCollator() const;
     DecimalFormatSymbols * initializeDecimalFormatSymbols(UErrorCode &status);
     const DecimalFormatSymbols * getDecimalFormatSymbols() const;

icu4j/main/core/src/main/java/com/ibm/icu/text/RuleBasedNumberFormat.java

@@ -1362,6 +1362,11 @@ public Number parse(String text, ParsePosition parsePosition) {
      */
     public void setLenientParseMode(boolean enabled) {
         lenientParse = enabled;
+        if (enabled) {
+            // Eagerly initialize the scanner provider so that getLenientScannerProvider()
+            // is thread-safe when called from parse().
+            initializeLenientScannerProvider();
+        }
     }
 
     /**
@@ -1398,10 +1403,20 @@ public void setLenientScannerProvider(RbnfLenientScannerProvider scannerProvider
      * @stable ICU 4.4
      */
     public RbnfLenientScannerProvider getLenientScannerProvider() {
-        // there's a potential race condition if two threads try to set/get the scanner at
-        // the same time, but you get what you get, and you shouldn't be using this from
-        // multiple threads anyway.
         if (scannerProvider == null && lenientParse && !lookedForScanner) {
+            initializeLenientScannerProvider();
+        }
+
+        return scannerProvider;
+    }
+
+    /**
+     * Attempts to instantiate the default lenient scanner provider. Called eagerly from
+     * setLenientParseMode() so that getLenientScannerProvider() does not need to perform lazy
+     * initialization from parse() (thread safety).
+     */
+    private void initializeLenientScannerProvider() {
+        if (scannerProvider == null && !lookedForScanner) {
             try {
                 lookedForScanner = true;
                 Class<?> cls = Class.forName("com.ibm.icu.impl.text.RbnfScannerProviderImpl");
@@ -1412,8 +1427,6 @@ public RbnfLenientScannerProvider getLenientScannerProvider() {
                 // any failure, we just ignore and return null
             }
         }
-
-        return scannerProvider;
     }
 
     /**
@@ -2066,10 +2079,12 @@ private String adjustForContext(String result) {
                     // should only happen when deserializing, etc.
                     capitalizationBrkIter = BreakIterator.getSentenceInstance(locale);
                 }
+                // Clone the break iterator to avoid mutating it (thread safety).
+                BreakIterator iter = (BreakIterator) capitalizationBrkIter.clone();
                 return UCharacter.toTitleCase(
                         locale,
                         result,
-                        capitalizationBrkIter,
+                        iter,
                         UCharacter.TITLECASE_NO_LOWERCASE
                                 | UCharacter.TITLECASE_NO_BREAK_ADJUSTMENT);
             }