ICU-23395 Fix out-of-bounds read in expandCompositCharAtNear()

[TristanInSec]

Add i < sourceLength - 1 bounds check before the dest[i+1] access in
expandCompositCharAtNear(). The existing loop bound allows i to reach
sourceLength-1, making dest[i+1] read one element past the allocation.

Checklist

• Required: Issue filed: ICU-23395
• Required: The PR title must be prefixed with a JIRA Issue number.
• Required: Each commit message must be prefixed with a JIRA Issue number.
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

[CLAassistant]

All committers have signed the CLA.

[jira-pull-request-webhook]

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

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

Please

• create a real Jira ticket
• restore & fill out the pull request template
• write a unit test that fails before the fix and passes with it

[TristanInSec]

Hi @markusicu,

Done:

• Created ICU-23395 for this issue specifically.
• Will restore and fill out the PR template.
• Will add a unit test that fails before the fix and passes after.

Note that this is a different bug class from the deserialization issues in #3961/#3962/#3964. The off-by-one in expandCompositCharAtNear() is triggered by normal Arabic text input to u_shapeArabic(), not by malformed binary data. The GIGO consideration does not apply here.

Best regards,
Tristan

[markusicu]

Hi @TristanInSec , thank you!

I started the CI checks, there are failures. Please take a look.

Also, we have a Java version of this API:
https://unicode-org.github.io/icu-docs/apidoc/dev/icu4j/com/ibm/icu/text/ArabicShaping.html

Would you mind taking a look at the Java code to see if it has similar buffer handling, and adding an equivalent unit test case?

[TristanInSec]

Hi @markusicu, thanks for the review!

I fixed the CI failures -- the test was incorrectly treating U_NO_SPACE_AVAILABLE as an error. The crafted input legitimately triggers the no-space path in expansion; the test's purpose is to verify no out-of-bounds access under ASan/Valgrind, not a specific return code. Updated to accept that outcome.

I also checked the Java ArabicShaping class. The Java expandCompositCharAtNear() is not affected -- it iterates backward with dest[i-1] guarded by i > start, so there is no equivalent off-by-one. I added a regression test in ArabicShapingRegTest.java anyway to guard against future changes.

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4j/main/core/src/test/java/com/ibm/icu/dev/test/shaping/ArabicShapingRegTest.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[TristanInSec]

Hi @markusicu,

Checked the Java counterpart. The Java expandCompositCharAtNear in ArabicShaping.java is not affected -- it iterates backward and only accesses dest[i - 1] with an i > start guard. The C version's forward dest[i+1] access without bounds check is what caused the OOB read.

The other dest[i + 1] accesses in deshapeNormalize() are already guarded by i < (length - 1), so those are safe too.

Could you re-trigger the CI on the latest commit when you get a chance? The test fix from the last push should resolve the earlier failures.

Thanks,
Tristan

[eggrobin]

Hi, apologies for the delayed response.

In general it would be a good idea to try to make your inputs as simple as possible so that we can actually tell what the desired behaviour would be in the test case. It should be possible (indeed it is possible, see below) to reproduce the issue on something simpler than your source[], which, contra the comment, is not an Arabic string, but a 86-code point mishmash of CJK, controls, noncharacter code points, and three Arabic characters sprinkled in (including a lone U+FEF5 which is probably why we are going through this lam-alef path in the first place).

In addition, this test case surely does not exercise an out-of-bounds read; dest in expandCompositCharAtNear ends up being this temporary buffer,

icu/icu4c/source/common/ushape.cpp

Line 1501 in 5650473

char16_t buffer[300];

which is comfortably larger than your sourceLength of 86.

I thought this might be an uninitialized read of buffer, but I am not seeing that either; when we read dest[i+1] with i==sourceLength -1 (=85 in your test), that has been set to 0, here:

icu/icu4c/source/common/ushape.cpp

Line 1590 in 5650473

uprv_memset(tempbuffer+sourceLength, 0, (outputSize-sourceLength)*U_SIZEOF_UCHAR);

So in your test case, that dest[i+1] when i==sourceLength -1 appears to be a well-defined read of in-bounds initialized memory (set to 0). Indeed that test case passes when I try to run it on the old code with address sanitizing enabled.

There does seem to be an out-of-bounds read when sourceLength=300 or sourceLength≥301 (two different paths, either past the stack buffer or past the heap-allocated tempBuffer).

Please write a test that actually exercises that out-of-bounds read (both paths), with a test string that would actually succeed so we don’t get confused by the error code. (299 or 300 spaces followed by a a lam-alef ligature will do the job).

[TristanInSec]

Thanks for the thorough review, you're absolutely right. I rewrote the test to exercise both buffer paths:

• sourceLength=300: 299 spaces + U+FEF5 (overreads the stack buffer[300])
• sourceLength=301: 300 spaces + U+FEF5 (overreads the heap-allocated buffer)

Both calls succeed without error. Updated the Java test with the same inputs for consistency.