Skip to content

fix undefined 32-bit shift in dwarf LEB128 decoding - #16565

Open
isl-Ramzi wants to merge 1 commit into
firebase:mainfrom
isl-Ramzi:dwarf-leb128-shift-width
Open

fix undefined 32-bit shift in dwarf LEB128 decoding#16565
isl-Ramzi wants to merge 1 commit into
firebase:mainfrom
isl-Ramzi:dwarf-leb128-shift-width

Conversation

@isl-Ramzi

Copy link
Copy Markdown
Contributor

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.

@gemini-code-assist

Copy link
Copy Markdown
Contributor
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

@paulb777

Copy link
Copy Markdown
Member

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

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);

Suggested change
result |= -((uint64_t)1 << shift);
result |= ~(((uint64_t)1 << shift) - 1);

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants