From 4ee829d343f4c144535d766f23e2be0f3250613b Mon Sep 17 00:00:00 2001
From: Andrew Paseltiner <[email protected]>
Date: Mon, 27 Apr 2026 16:29:19 -0700
Subject: [PATCH] Fix data race and interleaved mutation in base::FormatDouble

base::FormatDouble used a process-global ICU NumberFormat instance
without synchronization. This pattern was introduced in 2011
(https://codereview.chromium.org/7189076) and is fundamentally broken
for two reasons:

1. Interleaved Mutations: Because the global formatter was shared,
   concurrent calls from different threads could interleave their
   calls to setMaximumFractionDigits/setMinimumFractionDigits. This
   meant one thread's configuration could be overwritten by another
   before the formatting call occurred, leading to incorrect results.

2. Data Race/UAF: ICU formatters are not thread-safe for mutation.
   Concurrent mutations could trigger internal reallocations in ICU,
   leading to a heap-use-after-free in the browser process.

This CL replaces the global instances with base::ThreadLocalOwnedPointer
to ensure each thread has its own dedicated formatter.

Fixed: 506477192
Change-Id: Ib1649cb6fe320f2b97fd61e19134ba1e53974287
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7796498
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Daniel Cheng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1621408}
---

diff --git a/base/i18n/number_formatting.cc b/base/i18n/number_formatting.cc
index cd8bb78d..036768e 100644
--- a/base/i18n/number_formatting.cc
+++ b/base/i18n/number_formatting.cc
@@ -12,10 +12,12 @@
 #include "base/format_macros.h"
 #include "base/i18n/message_formatter.h"
 #include "base/i18n/unicodestring.h"
-#include "base/lazy_instance.h"
+#include "base/memory/ptr_util.h"
+#include "base/no_destructor.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/threading/thread_local.h"
 #include "third_party/icu/source/common/unicode/ustring.h"
 #include "third_party/icu/source/i18n/unicode/numfmt.h"
 
@@ -23,34 +25,45 @@
 
 namespace {
 
-// A simple wrapper around icu::NumberFormat that allows for resetting it
-// (as LazyInstance does not).
-struct NumberFormatWrapper {
-  NumberFormatWrapper() { Reset(); }
+// ICU formatters are not thread-safe for mutation. Using thread-local storage
+// ensures that each thread has its own instance, preventing data races and
+// potential Use-After-Free when multiple threads concurrently call
+// FormatDouble with different digit requirements. See crbug.com/506477192.
+base::ThreadLocalOwnedPointer<icu::NumberFormat>& GetIntFormatterStorage() {
+  static base::NoDestructor<base::ThreadLocalOwnedPointer<icu::NumberFormat>>
+      instance;
+  return *instance;
+}
 
-  void Reset() {
-    // There's no ICU call to destroy a NumberFormat object other than
-    // operator delete, so use the default Delete, which calls operator delete.
-    // This can cause problems if a different allocator is used by this file
-    // than by ICU.
-    UErrorCode status = U_ZERO_ERROR;
-    number_format.reset(icu::NumberFormat::createInstance(status));
-    DCHECK(U_SUCCESS(status));
+base::ThreadLocalOwnedPointer<icu::NumberFormat>& GetFloatFormatterStorage() {
+  static base::NoDestructor<base::ThreadLocalOwnedPointer<icu::NumberFormat>>
+      instance;
+  return *instance;
+}
+
+icu::NumberFormat* GetFormatter(
+    base::ThreadLocalOwnedPointer<icu::NumberFormat>& storage) {
+  icu::NumberFormat* formatter = storage.Get();
+  if (formatter) {
+    return formatter;
   }
-
-  std::unique_ptr<icu::NumberFormat> number_format;
-};
-
-LazyInstance<NumberFormatWrapper>::DestructorAtExit g_number_format_int =
-    LAZY_INSTANCE_INITIALIZER;
-LazyInstance<NumberFormatWrapper>::DestructorAtExit g_number_format_float =
-    LAZY_INSTANCE_INITIALIZER;
+  // There's no ICU call to destroy a NumberFormat object other than
+  // operator delete, so use the default Delete, which calls operator delete.
+  // This can cause problems if a different allocator is used by this file
+  // than by ICU.
+  UErrorCode status = U_ZERO_ERROR;
+  std::unique_ptr<icu::NumberFormat> instance(
+      icu::NumberFormat::createInstance(status));
+  DCHECK(U_SUCCESS(status));
+  formatter = instance.get();
+  storage.Set(std::move(instance));
+  return formatter;
+}
 
 }  // namespace
 
 std::u16string FormatNumber(int64_t number) {
-  icu::NumberFormat* number_format =
-      g_number_format_int.Get().number_format.get();
+  icu::NumberFormat* number_format = GetFormatter(GetIntFormatterStorage());
 
   if (!number_format) {
     // As a fallback, just return the raw number in a string.
@@ -69,8 +82,7 @@
 std::u16string FormatDouble(double number,
                             int min_fractional_digits,
                             int max_fractional_digits) {
-  icu::NumberFormat* number_format =
-      g_number_format_float.Get().number_format.get();
+  icu::NumberFormat* number_format = GetFormatter(GetFloatFormatterStorage());
 
   if (!number_format) {
     // As a fallback, just return the raw number in a string.
@@ -90,8 +102,8 @@
 }
 
 void ResetFormattersForTesting() {
-  g_number_format_int.Get().Reset();
-  g_number_format_float.Get().Reset();
+  GetIntFormatterStorage().Set(nullptr);
+  GetFloatFormatterStorage().Set(nullptr);
 }
 
 }  // namespace base
diff --git a/base/i18n/number_formatting_unittest.cc b/base/i18n/number_formatting_unittest.cc
index 78afd6a2..61fc0dd 100644
--- a/base/i18n/number_formatting_unittest.cc
+++ b/base/i18n/number_formatting_unittest.cc
@@ -8,10 +8,14 @@
 #include <stdint.h>
 
 #include <limits>
+#include <vector>
 
 #include "base/i18n/rtl.h"
+#include "base/memory/raw_ptr.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/test/icu_test_util.h"
+#include "base/threading/simple_thread.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/icu/source/i18n/unicode/usearch.h"
@@ -19,6 +23,29 @@
 namespace base {
 namespace {
 
+class NumberFormatWorkerThread : public base::SimpleThread {
+ public:
+  NumberFormatWorkerThread(base::WaitableEvent* event,
+                           int digits,
+                           int iterations)
+      : SimpleThread("NumberFormatWorkerThread"),
+        event_(event),
+        digits_(digits),
+        iterations_(iterations) {}
+
+  void Run() override {
+    event_->Wait();
+    for (int i = 0; i < iterations_; ++i) {
+      FormatDouble(1.2345678, digits_);
+    }
+  }
+
+ private:
+  const raw_ptr<base::WaitableEvent> event_;
+  const int digits_;
+  const int iterations_;
+};
+
 TEST(NumberFormattingTest, FormatNumber) {
   static const struct {
     int64_t number;
@@ -180,5 +207,27 @@
   }
 }
 
+// Regression test for crbug.com/506477192. Ensure that concurrent calls to
+// FormatDouble with different fractional digits don't cause a data race in
+// the shared ICU number formatter.
+TEST(NumberFormattingTest, FormatDoubleRace) {
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  std::vector<std::unique_ptr<NumberFormatWorkerThread>> threads;
+
+  for (int i = 0; i < 20; ++i) {

diff --git a/base/i18n/number_formatting_unittest.cc b/base/i18n/number_formatting_unittest.cc
index 78afd6a2..61fc0dd 100644
--- a/base/i18n/number_formatting_unittest.cc
+++ b/base/i18n/number_formatting_unittest.cc
@@ -8,10 +8,14 @@
 #include <stdint.h>
 
 #include <limits>
+#include <vector>
 
 #include "base/i18n/rtl.h"
+#include "base/memory/raw_ptr.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/test/icu_test_util.h"
+#include "base/threading/simple_thread.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/icu/source/i18n/unicode/usearch.h"
@@ -19,6 +23,29 @@
 namespace base {
 namespace {
 
+class NumberFormatWorkerThread : public base::SimpleThread {
+ public:
+  NumberFormatWorkerThread(base::WaitableEvent* event,
+                           int digits,
+                           int iterations)
+      : SimpleThread("NumberFormatWorkerThread"),
+        event_(event),
+        digits_(digits),
+        iterations_(iterations) {}
+
+  void Run() override {
+    event_->Wait();
+    for (int i = 0; i < iterations_; ++i) {
+      FormatDouble(1.2345678, digits_);
+    }
+  }
+
+ private:
+  const raw_ptr<base::WaitableEvent> event_;
+  const int digits_;
+  const int iterations_;
+};
+
 TEST(NumberFormattingTest, FormatNumber) {
   static const struct {
     int64_t number;
@@ -180,5 +207,27 @@
   }
 }
 
+// Regression test for crbug.com/506477192. Ensure that concurrent calls to
+// FormatDouble with different fractional digits don't cause a data race in
+// the shared ICU number formatter.
+TEST(NumberFormattingTest, FormatDoubleRace) {
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  std::vector<std::unique_ptr<NumberFormatWorkerThread>> threads;
+
+  for (int i = 0; i < 20; ++i) {
+    threads
+        .emplace_back(
+            std::make_unique<NumberFormatWorkerThread>(&event, i % 5, 1000))
+        ->Start();
+  }
+
+  event.Signal();
+
+  for (auto& t : threads) {
+    t->Join();
+  }
+}
+
 }  // namespace
 }  // namespace base