ICU-23485 Fix stack-buffer-underflow in _cmpFold

[FrankYFTang]

Fix generated by AI

Summary

Fixes a critical Out-of-Bounds (OOB) Stack Read Underflow (fold1[-1] / fold2[-1]) in the core _cmpFold case-insensitive Unicode string comparison implementation (ustrcase.cpp).

Root Cause

During look-behind operations for trail-surrogate case-folding expansions:

• When a target string matches from its primary level 0 source, while the other string is actively emitting from a level 1 stack-allocated fold1/fold2 decomposition buffer.
• The algorithm attempted to "rewind" the evaluation cursor using an unguarded pre-decrement and immediate look-behind read (--s; ... c = *(s-1);).
• If the pre-decrement aligns the pointer precisely with the zero-index origin of the active fold array (fold[0]), the *(s-1) dereference targets address space positioned exactly 2 bytes prior to the fold local buffer boundary.
• Under AddressSanitizer (ASAN), this results in an immediate Stack-Buffer-Overflow (Read Underflow) SIGABRT.

Solution / Patch Architecture

1. Safety Enveloping: Enforces strict s - start >= 2 bounds-validation prior to pointer arithmetic. Look-behinds evaluating below the active buffer's start origin are safely clamped directly to start.
2. Fetch-Loop Delegation (c = -1): Replaces the unguarded and unsafe *(s-1) pointer dereferences entirely by assigning -1 to the respective comparative register (c1/c2). This redirects the look-behind fetch operation to _cmpFold's existing, bounds-checked *s++ fetch-and-increment machinery on the subsequent for(;;) loop iteration.
3. Prefix-Tracker (m) Origin Protection: Wraps --m2 reductions inside strict m > org boundary validations to prevent secondary underflows of the m1/m2 string-prefix length cursors.

Testing & Validation

• strcase.cpp (New Unit Test: TestCmpFoldStackBufferUnderflow): Implements an ICU intltest C++ regression routine utilizing the exact multi-level UTF-16 surrogate alignment sequence isolated from the V8 d8 (/ui RegExp backreference) vulnerability report.
• Validates that u_strcmpFold comparison logic completes deterministically and reports U_ZERO_ERROR.
• Executes with 0 AddressSanitizer (ASAN) memory-safety violations or fold1 stack-frame warnings under --config=asan.
  Jira Ticket Link: ICU-23485

Checklist

• Required: Issue filed: ICU-23485
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable
• Approver: Feel free to merge on my behalf

[FrankYFTang]

To understand why c2 = -1; (and c1 = -1;) is the correct and necessary assignment, we have to look at the state-machine logic governing _cmpFold's main comparison loop.

1. Pointer vs. Comparison Register
   s1 and s2 are indeed the pointers (const char16_t*). They act as the "read-heads" traversing through the UTF-16 string buffers (or fold1/fold2).
   c1 and c2 are 32-bit signed integers (int32_t). They act as "data registers" holding the currently active code-unit being compared, OR the special control-value -1.
2. The _cmpFold Fetch Protocol (if (c < 0))
   Look closely at how the outer for(;;) comparison loop in _cmpFold handles data-retrieval (Lines 1555–1566):

cpp


if(c2<0) {
    // If c2 is negative, it triggers a fetch from the *s2 pointer 
    // AND automatically increments the s2 pointer (++s2).
    c2 = *s2++; 
}

• If c2 contains a valid Unicode code-unit (>= 0), the loop skips the fetch logic and uses the existing c2 value for comparison.
• If c2 is explicitly set to -1, it acts as a "Fetch Required" flag. This forces the loop to dereference s2 and grab the next character on its very next iteration.

3. Why c2 = -1; Replaces c2 = *(s2-1);
   When the case-folding logic triggers a look-behind, the algorithm needs to rewind the comparison stream so that the next comparison operation evaluates the lead surrogate of the previous pair.

The Original (Vulnerable) Code:

Executed --s2; to move the pointer back.
Executed c2 = *(s2-1); to immediately pre-load the lead-surrogate into c2, bypassing the loop's c2 < 0 fetch block.
The Bug: Because of the --s2 state, evaluating *(s2-1) resulted in a fold2[-1] OOB stack-read if s2 was already at the buffer's origin.

The Memory-Safe Patch:

Safely positions the s2 pointer to the exact beginning of the desired surrogate (clamped to start2).
Assigns c2 = -1;.
The Result: On the immediate next pass of the for(;;) loop, the if (c2 < 0) condition evaluates to True. The loop safely executes c2 = *s2++, retrieving the exact code-unit we just rewound to—utilizing the existing, fully bounds-checked, and post-incremented fetch infrastructure of ICU.

Summary

Setting c2 = -1 (and c1 = -1) leverages the native state-machine of the _cmpFold loop to perform the look-behind read implicitly and safely via the existing *s2++ architecture, entirely eliminating the need for *(s-1) pointer-subtraction arithmetic.

[FrankYFTang]

I am still reviewing the PR. Please ignore it for now

[markusicu]

Please ignore it for now

Let us know when it's ready (and remove the "incomplete" label).

[roubert]

Please ignore it for now

Let us know when it's ready (and remove the "incomplete" label).

It's possible to click Convert to draft on a GitHub PR (upper right corner in the UI, just below the list of reviewers) to clearly signal to humans and machines alike that a PR is not yet ready. It can also be helpful to not assign any reviewers until a PR is actually ready for review.

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[FrankYFTang]

please review now. thanks

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4c/source/test/intltest/strcase.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[FrankYFTang]

PTAL

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4c/source/test/intltest/strcase.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot