fix undefined 32-bit shift in dwarf LEB128 decoding - #16565
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves potential undefined behavior in DWARF data parsing by casting shift operands to uint64_t in FIRCLSParseULEB128AndAdvance and FIRCLSParseLEB128AndAdvance, and adds corresponding unit tests for large values. The reviewer feedback highlights that the parsing loops are still limited to 8 iterations instead of the 10 bytes required for full 64-bit LEB128/SLEB128 decoding, which could lead to truncation. Additionally, a cleaner bitwise NOT alternative is suggested to replace the unary minus on an unsigned operand to prevent compiler warnings.
| *cursor += 1; | ||
|
|
||
| result |= ((0x7F & byte) << shift); | ||
| result |= ((uint64_t)(0x7F & byte) << shift); |
There was a problem hiding this comment.
While widening the shift operand to uint64_t correctly prevents undefined behavior for shifts >= 32, the loop itself is still limited to sizeof(uint64_t) (8) iterations.
A 64-bit integer encoded in LEB128 can require up to 10 bytes (since ceil(64 / 7) = 10). Limiting the loop to 8 iterations means any ULEB128 value >= 2^56 will be truncated, and the cursor will not be advanced past the remaining bytes, which will corrupt subsequent parsing of the DWARF stream.
Consider increasing the loop limit to 10 (or defining a constant for the maximum LEB128 bytes).
| *cursor += 1; | ||
|
|
||
| result |= ((0x7F & byte) << shift); | ||
| result |= ((uint64_t)(0x7F & byte) << shift); |
There was a problem hiding this comment.
Similar to FIRCLSParseULEB128AndAdvance, the loop here is limited to sizeof(uint64_t) (8) iterations. A 64-bit signed integer encoded in SLEB128 can require up to 10 bytes.
Limiting the loop to 8 iterations prevents correct decoding of large values and leaves the cursor in an incorrect state for subsequent DWARF parsing.
Consider increasing the loop limit to 10.
| if ((shift < size) && (0x40 & byte)) { | ||
| // sign extend | ||
| result |= -(1 << shift); | ||
| result |= -((uint64_t)1 << shift); |
There was a problem hiding this comment.
Applying the unary minus operator to an unsigned operand ((uint64_t)1 << shift) is well-defined in C, but it can trigger compiler warnings or static analysis alerts (e.g., about unary minus on unsigned types).
A cleaner, warning-free alternative to generate the sign-extension mask is using bitwise NOT:
result |= ~(((uint64_t)1 << shift) - 1);
| result |= -((uint64_t)1 << shift); | |
| result |= ~(((uint64_t)1 << shift) - 1); |
Repro: unwind a frame whose __eh_frame CFI record holds a ULEB128/SLEB128 operand that needs five or more bytes (any value at or above 2^28), e.g. an augmentation length or DWARF expression length.
Cause: FIRCLSParseULEB128AndAdvance and FIRCLSParseLEB128AndAdvance accumulate into a uint64_t but shift the masked byte as a 32-bit int, so the shift count climbs to 49 (ULEB) and 56 (the SLEB sign-extension). Shifting an int by its width or more is undefined; on arm64 the count is masked to 5 bits, so a ULEB128 of 0xFFFFFFFF decodes to 0xFFFFFFFFFFFFFFFF and an SLEB128 of -2^32 to -8. That value then feeds pointer/length math on the record.
Fix: widen the shifted operand (and the sign-extension constant) to uint64_t at the three sites. Values below 2^28 decode exactly as before; larger ones now match the spec. Tests in FIRCLSDwarfTests.m cover a 5-byte ULEB128 and a 5-byte negative SLEB128.