New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
This site will be read-only for 3-4 hours starting at Sunday, 08:00AM PDT
Starred by 2 users

Issue metadata

Status: Fixed
Closed: Dec 2012
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Sign in to add a comment

Heap-use-after-free in icu_46::RuleBasedCollator::RuleBasedCollator

Project Member Reported by, Sep 26 2012 Back to list

Issue description

Detailed report:

Fuzzer: Inferno_twister

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x7ff3ef2c5aa8
Crash State:
  - crash stack -
  - free stack -
Status: Assigned
looks like a race condition in ICU.

Comment 2 by, Sep 26 2012

Labels: Feature-Fonts

Comment 3 by, Sep 26 2012

Labels: -Area-WebKit -Feature-Fonts Area-Internals
Its in ICU.  I've deduced the following by reading the code.

Threads T0 and T6 call locale::GetDefault() in locid.cpp at the same time, which is supposed to be thread safe.  But both can call: 

665    UMTX_CHECK(NULL, gDefaultLocale, retLocale); 

before either gets a chance to create gDefaultLocale, since the mutex is dropped at the end of the CHECK call.  Both would then try to call: 

667   locale_set_default_internal(NULL);

to set the same (default-named locale).  One thread will win at 

162         umtx_lock(NULL);
163         if (gDefaultLocale == NULL) {
164             gDefaultLocale = newFirstDefault;  // Assignment to gDefaultLocale must happen inside mutex

and subsequently bail at line 173, returning the first version of the locale to the caller.  This will be the one that eventually gets cleared by the second thread causing the problem.

The second thread continues at:

179    UBool hashTableNeedsInit;
180    UMTX_CHECK(NULL, (gDefaultLocalesHashT == NULL), hashTableNeedsInit);
181    if (hashTableNeedsInit) {

and creates the hashtable, but doesn't insert the existing entry into it.  Instead, insertion is deferred until line 277 for reasons exceeding my comprehension.

Hence the test  

201    Locale *newDefault = (Locale *)uhash_get(gDefaultLocalesHashT, localeNameBuf);      
202    if (newDefault != NULL) {

flunks, because even though the first-created locale suits our purposes, it isn't in the table yet.  So the thread makes a duplicate locale and starts to insert it into the table, but realizes that inserting the first has been deferred and inserts the first at line 227 as mentioned earlier:

224             if (hashTableNeedsInit) {
225                 // This is the second request to set the locale.
226                 // Cache the first one.
227                 uhash_put(gDefaultLocalesHashT, (void *)gDefaultLocale->getName(), gDefaultLocale, &status);
228             }

Then it inserts the duplicate (with the same key), and as part of replacement, the hash table invokes its deleter to delete the old entry (the first locale).  

The most mindless fix would be to perform the insertion into the hash table not at the block at line 227, but at line 190 or so following the allocation of the hash table, but before the mutex is dropped.

There's still the problem in that case that there is a hash table when all that exists is a single locale, which was the optimization this code seems trying to avoid by the convoluted dance.  One could do a key comparison against gDefaultLocale at line 177 and return if it matches, avoiding the situation in this case, but making the general case more expensive, so I'd avoid doing that and just bite the bullet in this rare situation.

I can cobble together a patch, but I'm not sure how one would ever test it.

Comment 4 by, Sep 26 2012

Index: third_party/icu/source/common/locid.cpp
--- third_party/icu/source/common/locid.cpp	(revision 158118)
+++ third_party/icu/source/common/locid.cpp	(working copy)
@@ -189,9 +189,10 @@
         if (gDefaultLocalesHashT == NULL) {
             gDefaultLocalesHashT = tHashTable;
             ucln_common_registerCleanup(UCLN_COMMON_LOCALE, locale_cleanup);
+            // Cache the first one.
+            uhash_put(gDefaultLocalesHashT, (void *)gDefaultLocale->getName(), gDefaultLocale, &status);
         } else {
-            hashTableNeedsInit = FALSE;
@@ -221,11 +222,6 @@
         Locale *hashTableVal = (Locale *)uhash_get(gDefaultLocalesHashT, key);
         if (hashTableVal == NULL) {
-            if (hashTableNeedsInit) {
-                // This is the second request to set the locale.
-                // Cache the first one.
-                uhash_put(gDefaultLocalesHashT, (void *)gDefaultLocale->getName(), gDefaultLocale, &status);
-            }
             uhash_put(gDefaultLocalesHashT, (void *)key, newDefault, &status);
             gDefaultLocale = newDefault;
             // ignore errors from hash table insert.  (Couldn't do anything anyway)

Labels: Mstone-22 SecImpacts-Stable SecImpacts-Beta
@Jshin, please look at Tom's patch and see if we can land this ICU change. 
 Issue 159369  has been merged into this issue.
 Issue 124795  has been merged into this issue.

Comment 8 by, Nov 12 2012

Sorry again that I didn't get back to this sooner. With my somewhat limited understanding of the code in question (after reading the code myself) and based on tsepez's explanation, the patch looks good.

Let's get this in unless inferno and others see a problem. 

And, I made a critical mistake of filing a bug against the upsteam believing that their 'sensitive' check box do what it's supposed to do (make it invisible to random people), but it does not. I'm really sorry about this. I've changed the contents, but they have a history still visible. I'm talking to the person in charge to get rid of the history completely. 

Comment 9 by, Nov 12 2012

Phew. The ICU trac maintainer removed the bug and its history. 

Comment 10 by, Nov 12 2012

argh... it's sent to the bug mailing list. I'm really sorry. 

 I'm making a CL with tsepez's patch. 

Comment 11 by, Nov 20 2012

tsepez and inferno : is a CL. Aren't you interested in taking that? An alternative is to take the upstream CL (they have made a comprehensive change there). 


I think we should pull in the upstream CL. Chris?
"comprehensive" does sound like a nice word?
What was the concensus here ? Lets do one of the options and close this.
Labels: -Mstone-22 Mstone-23
Moving all milestone 22 bugs to milestone 23
@jshin - please take the upstream CL and let us know when that has landed.
Labels: ReleaseBlock-Stable

Comment 19 by, Dec 3 2012

is this a blocker for m23? We're planning on cutting the final 23 on Tuesday.
Labels: -Mstone-23 -ReleaseBlock-Stable Mstone-24
No, it's not a blocker -- removed label.

It does seem to be dragging on a bit though.

Any update @jshin?
Sorry for the delay. The upstream change has been through a few iterations and it's now settled. I'll take it and apply to the Chrome tree. 

I've just tried to back-port the upstream patch to our copy, but the upstream change uses a newly introduced UMutex that we don't have. Perhaps, I can backport it, too, but it's getting uglier. 

I'm planning to update ICU to 51 in January. In the meantime, should we just apply tsepez's patch at : ? 

An alternative is to use the initial patch in the upstream that does not use UMutex. 

It turned out that the alternative to tsepez's patch (the initial upstream patch) also relies on UMutex. How about just going with tsepez's patch for now?

Ok, let's land my patch. JShin, please land CL
Tom, looks like Jshin is waiting for your lgtm and cq+ button on I don't know this code enough to lgtm.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased
Project Member

Comment 27 by, Dec 13 2012

The following revision refers to this bug:

r172827 | | 2012-12-13T05:50:35.649052Z

Changed paths:

Fix a race condition in ICU's locid.

Patch by tsepez.

The upstream bug is and the
upstream has a different patch than this, but we can't take it now because
it uses UMutex not available in Chrome's copy of ICU. We'll get it when
we upgrade to 50.1

BUG= 152442 
TEST=No more 'heap-use-after-free' in ICU from clusterfuzz as shown in the bug report.

Review URL:
Project Member

Comment 28 by, Dec 18 2012

The following revision refers to this bug:

r173696 | | 2012-12-18T09:33:32.003297Z

Changed paths:


BUG= 152442 

Review URL:
This missed the M25 branch point. I'll merge there and possibly to M24.
Status: Fixed
Project Member

Comment 31 by, Jan 11 2013

Labels: -Merge-Approved merge-merged-1364
The following revision refers to this bug:

r32387 | | 2013-01-11T03:03:11.502125Z

Labels: -Mstone-24 Mstone-25 Release-0
Rolled icu46 DEPS on M25 branch at src-internal @r32387

Also twiddling down severity to Medium.
Labels: -SecSeverity-High SecSeverity-Medium
Labels: CVE-2013-0900
Project Member

Comment 35 by, Mar 10 2013

Labels: -Area-Internals -Type-Security -SecSeverity-Medium -Stability-AddressSanitizer -SecImpacts-Stable -SecImpacts-Beta -Mstone-25 Security-Impact-Stable Security-Impact-Beta Security-Severity-Medium Cr-Internals Performance-Memory-AddressSanitizer Type-Bug-Security M-25
Labels: -Restrict-View-SecurityNotify
Project Member

Comment 37 by, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 38 by, Mar 21 2013

Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member

Comment 39 by, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 40 by, Apr 1 2013

Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer

Comment 41 by, Jun 4 2013

CCing David Belcher from RIM so that they can check if they are affected.
Project Member

Comment 42 by, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 43 by, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member

Comment 44 by, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment