Skip to content
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.

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

elftools: StringTableSection: Check table size before returning string #528

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vapier
Copy link
Contributor

@vapier vapier commented Dec 8, 2023

Some ELF files have strings pointing to an offset outside the string table dimension, let's throw an exception in that case.

Bug-Url: https://crbug.com/788925
Reviewed-on: https://chromium-review.googlesource.com/792553

Some ELF files have strings pointing to an offset outside the string
table dimension, let's throw an exception in that case.

Bug-Url: https://crbug.com/788925
Reviewed-on: https://chromium-review.googlesource.com/792553
Copy link
Owner

@eliben eliben left a comment

Choose a reason for hiding this comment

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

Adding a unit test could be nice here

Otherwise LGTM

@sevaa any objections?

@sevaa
Copy link
Contributor

sevaa commented Dec 11, 2023

The second assert is unnecessary - parse_cstring_from_stream will not read past the end of the stream (this ain't C, people). If there is indeed an unterminated string at the end of the section, parse_cstring_from_stream will return None and the expression in the assert will throw an exception that you are calling len() on a None object.

Remove the second check, it's pointless and wrong.

@sevaa
Copy link
Contributor

sevaa commented Apr 18, 2024

@vapier Are you still interested in this change?

@sevaa
Copy link
Contributor

sevaa commented Sep 23, 2024

Now that I look at it, should we really spend time on providing descriptive exceptions for various flavors of corrupt data? As things stand right now, pyelftools will exception on a file with a string offset leading to nowhere (decode called on NoneType will be the exception); with this patch, the exception will be more descriptive. On one hand, that's a user friendly thing to do; on the other hand, given the myriad ways an extremely interlinked data structure can be corrupt, should we endeavor to catalogue them all?

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.

4 participants