Skip to content

ICU-23352 Fix race condition UAF in uprv_collation_root_cleanup#3921

Open
hirorogo wants to merge 1 commit intounicode-org:maint/maint-54from
hirorogo:fix/uaf-002-collationroot
Open

ICU-23352 Fix race condition UAF in uprv_collation_root_cleanup#3921
hirorogo wants to merge 1 commit intounicode-org:maint/maint-54from
hirorogo:fix/uaf-002-collationroot

Conversation

@hirorogo
Copy link
Copy Markdown

@hirorogo hirorogo commented Mar 31, 2026

Summary

uprv_collation_root_cleanup() in collationroot.cpp calls SharedObject::clearPtr(rootSingleton) before initOnce.reset(). A concurrent getRoot() call can pass through umtx_initOnce() (seeing initOnce as already complete) and then dereference rootSingleton->tailoring after it has been set to NULL or freed.

Fix: Reset initOnce before clearing rootSingleton so concurrent callers block on re-initialization instead of accessing a dangling pointer.

PoC

// Reproduction: race condition in uprv_collation_root_cleanup
// Thread A calls cleanup, Thread B calls getRoot concurrently.
#include <cstdio>
#include <thread>
#include <atomic>
#include <chrono>

struct SharedObject {
    int tailoring;
    std::atomic<int> refcount{1};
};

std::atomic<bool> initDone{true};
SharedObject* rootSingleton = nullptr;

// Simulates buggy cleanup order
void cleanup_buggy() {
    // BUG: clear pointer first, reset init flag second
    SharedObject* old = rootSingleton;
    rootSingleton = nullptr;  // clearPtr
    if (old) old->refcount--;
    // ... time passes ...
    initDone.store(false);  // initOnce.reset() — too late!
}

// Simulates concurrent getRoot
void getRoot_buggy() {
    if (initDone.load()) {  // sees true (not yet reset)
        // Tries to access rootSingleton which is already NULL
        if (rootSingleton) {
            printf("  tailoring=%d\n", rootSingleton->tailoring);
        } else {
            printf("  BUG: rootSingleton is NULL — UAF/NPD if timing differs\n");
        }
    }
}

// Fixed: reset init flag first
void cleanup_fixed() {
    initDone.store(false);  // initOnce.reset() FIRST
    SharedObject* old = rootSingleton;
    rootSingleton = nullptr;
    if (old) old->refcount--;
}

void getRoot_fixed() {
    if (initDone.load()) {  // sees false -> blocks/reinits
        printf("  accessing rootSingleton\n");
    } else {
        printf("  FIXED: initOnce not done, will re-initialize\n");
    }
}

int main() {
    SharedObject obj{42};
    rootSingleton = &obj;
    initDone = true;

    printf("=== BUGGY: race window exists ===\n");
    std::thread t1(cleanup_buggy);
    std::thread t2(getRoot_buggy);
    t1.join(); t2.join();

    rootSingleton = &obj;
    initDone = true;

    printf("=== FIXED: no race window ===\n");
    std::thread t3(cleanup_fixed);
    std::this_thread::sleep_for(std::chrono::milliseconds(1));
    std::thread t4(getRoot_fixed);
    t3.join(); t4.join();

    return 0;
}

Test plan

  • Existing ICU collation tests pass
  • Concurrent cleanup + getRoot() no longer races on rootSingleton

Reset initOnce before clearing rootSingleton so that concurrent
getRoot()/getRootCacheEntry() callers block on re-initialization
instead of dereferencing a null or freed rootSingleton pointer.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hirorogo hirorogo changed the title Fix race condition UAF in uprv_collation_root_cleanup ICU-23352 Fix race condition UAF in uprv_collation_root_cleanup Apr 1, 2026
@hirorogo
Copy link
Copy Markdown
Author

hirorogo commented Apr 1, 2026

Could a maintainer please transition the Jira ticket ICU-23352 from New to Accepted? The jira-ticket CI check requires the ticket to be in "Accepted" status, but as a new contributor I don't have permission to change the ticket status. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant