Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
isl-Ramzi wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDataParsing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ uint64_t FIRCLSParseULEB128AndAdvance(const void** cursor) {

*cursor += 1;

result |= ((0x7F & byte) << shift);
result |= ((uint64_t)(0x7F & byte) << shift);

@gemini-code-assist gemini-code-assist Bot Aug 28, 2026

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

if ((0x80 & byte) == 0) {
break;
}
Expand All @@ -120,7 +120,7 @@ int64_t FIRCLSParseLEB128AndAdvance(const void** cursor) {

*cursor += 1;

result |= ((0x7F & byte) << shift);
result |= ((uint64_t)(0x7F & byte) << shift);

@gemini-code-assist gemini-code-assist Bot Aug 28, 2026

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.

shift += 7;

/* sign bit of byte is second high order bit (0x40) */
Expand All @@ -131,7 +131,7 @@ int64_t FIRCLSParseLEB128AndAdvance(const void** cursor) {

if ((shift < size) && (0x40 & byte)) {
// sign extend
result |= -(1 << shift);
result |= -((uint64_t)1 << shift);

@gemini-code-assist gemini-code-assist Bot Aug 28, 2026

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

}

return result;
Expand Down
20 changes: 20 additions & 0 deletions Crashlytics/UnitTests/FIRCLSDwarfTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "Crashlytics/Crashlytics/Components/FIRCLSContext.h"
#include "Crashlytics/Crashlytics/Components/FIRCLSGlobals.h"
#include "Crashlytics/Crashlytics/Helpers/FIRCLSDefines.h"
#include "Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDataParsing.h"
#include "Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfUnwind.h"
#include "Crashlytics/Crashlytics/Unwind/FIRCLSUnwind_arch.h"

Expand Down Expand Up @@ -172,6 +173,25 @@ - (void)testAssignReturnRegisterNumber {
XCTAssertEqual(FIRCLSDwarfUnwindGetRegisterValue(&outputRegisters, CLS_DWARF_REG_RETURN), 777);
}

- (void)testParseULEB128DecodesValueWiderThan28Bits {
// ULEB128 of 0xFFFFFFFF needs 5 bytes; the fifth byte lands at shift 28, so
// decoding the 0x7F chunk as a 32-bit int shifts past the sign bit.
const uint8_t encoded[] = {0xFF, 0xFF, 0xFF, 0xFF, 0x0F};
const void* cursor = encoded;

XCTAssertEqual(FIRCLSParseULEB128AndAdvance(&cursor), 0xFFFFFFFFULL);
XCTAssertEqual(cursor, (const void*)(encoded + sizeof(encoded)));
}

- (void)testParseLEB128DecodesLargeNegativeValue {
// SLEB128 of -2^32; the sign-extension and the value chunks both shift past 32.
const uint8_t encoded[] = {0x80, 0x80, 0x80, 0x80, 0x70};
const void* cursor = encoded;

XCTAssertEqual(FIRCLSParseLEB128AndAdvance(&cursor), -4294967296LL);
XCTAssertEqual(cursor, (const void*)(encoded + sizeof(encoded)));
}

#endif

@end
Loading