Skip to content

Parse .ARM.attributes for Thumb hints - #8633

Open
saagarjha wants to merge 1 commit into
binaryreader_leb128from
test_arm_attributes
Open

saagarjha wants to merge 1 commit into
binaryreader_leb128from
test_arm_attributes

Conversation

@saagarjha

Copy link
Copy Markdown
Contributor

Some stripped Thumb binaries have an even entrypoint, which causes us to analyze them as ARM and have a hard time finding any functions at all. .ARM.attributes, if present, can occasionally help us avoid this case by providing yet another clue that the binary is intended to be run as Thumb from the get-go.

@saagarjha
saagarjha force-pushed the test_arm_attributes branch from 49e1b34 to ba220ec Compare October 6, 2026 06:51
@saagarjha
saagarjha force-pushed the binaryreader_leb128 branch from b48131d to 165c6aa Compare October 6, 2026 06:52
Some stripped Thumb binaries have an even entrypoint, which causes us to
analyze them as ARM and have a hard time finding any functions at all.
.ARM.attributes, if present, can occasionally help us avoid this case by
providing yet another clue that the binary is intended to be run as
Thumb from the get-go.
@saagarjha
saagarjha force-pushed the binaryreader_leb128 branch from 165c6aa to bc56277 Compare October 6, 2026 13:43
@saagarjha
saagarjha force-pushed the test_arm_attributes branch from ba220ec to afe5f96 Compare October 6, 2026 14:01
An error occurred while trying to automatically change base from binaryreader_leb128 to binaryreader_slice October 6, 2026 14:04
An error occurred while trying to automatically change base from binaryreader_leb128 to binaryreader_slice October 6, 2026 14:04
An error occurred while trying to automatically change base from binaryreader_leb128 to binaryreader_slice October 6, 2026 14:04
An error occurred while trying to automatically change base from binaryreader_leb128 to binaryreader_slice October 6, 2026 14:09

@plafosse plafosse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving but I'd like to have a firm answer on what we should do with that one length check.

Comment thread view/elf/elfview.cpp
}
length -= sizeof(length);

auto subsection = section.Slice(section.GetOffset(), length);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We probably need a length check here? But maybe not as we're already in a try/catch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about this a bit and I think I would keep it as-is. If we have a bad length here, that reaches out beyond the extent of the section, then the call to slice will clamp it to the section's bounds. This is obviously only the case for invalid binaries.

I think the question here is what we want to do with those. I would argue that we should try our best to parse them anyway, i.e. be a little lenient with their sections, since we mostly are not going to be reading most of them anyway–we are only here for one attribute. I think it would be kind of annoying if Binary Ninja just bailed out of it saw something it didn't like (but a human could typically be willing to overlook): we are not a correctness validator. There are some cases where we genuinely cannot proceed because the corruption is making it impossible for us to do our job, but in this case, we just ignore the bad data and keep going. At least as far as I can tell.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is reasonable

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants