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

Return an empty file range for SHT_NOBITS sections #253

Merged
merged 2 commits into from Jan 31, 2021

Conversation

Tiwalun
Copy link
Contributor

@Tiwalun Tiwalun commented Jan 24, 2021

ELF sections with type SHT_NOBITS only exist in memory, and
don't contain any data in the ELF file itself. The range
returned by file_range() should be empty for such sections.

The previously returned range was wrong, and overlapped
the file range of the next section.

Fixes #252.

ELF sections with type SHT_NOBITS only exist in memory, and
don't contain any data in the ELF file itself. Fixes m4b#252.
// Sections with type SHT_NOBITS have no data in the file itself,
// they only exist in memory.
if self.sh_type == SHT_NOBITS {
self.sh_offset as usize..self.sh_offset as usize
Copy link
Owner

Choose a reason for hiding this comment

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

is returning a 0 range going to cause issues? ideally we'd return an Option<Range<usize>> i think, but that would be breaking change. Might be worth it though?

Copy link
Owner

Choose a reason for hiding this comment

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

ok i don't think this should be self.sh_offset..self.sh_offset; if sh_offset is non-zero you can get e.g., 10..10 or whatever, that is an empty range but will cause a panic if it's larger than buffer, i.e., this panics:

    let foo = [1, 2, 3];
    let x = &foo[10..10];

So i think it's better to return 0..0 ? or Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't expect the empty range to panic when it's out of bounds.

I think 0..0would be confusing as well, if someone looks at the start field of the range to decide where the data in the file starts.

An Option would be the solution, in my opinion.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok let's make it option, it's the right thing to do :) this will be a breaking change so unless you are in urgent need of a release, i may wait a bit to roll up other potential changes, or make other breaking changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't need it urgently, manually checking the section type is an easy workaround.

Some sections don't occupy any space in the file, for
these sections we now return None instead of a range.
Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

if you want to fix the typo that would be great, otherwise LGTM

pub fn file_range(&self) -> Range<usize> {
self.sh_offset as usize..(self.sh_offset as usize).saturating_add(self.sh_size as usize)
/// Returns this section header's file offset range,
/// if the section occupies space in fhe file.
Copy link
Owner

Choose a reason for hiding this comment

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

typo: fhe -> the

@m4b m4b merged commit ffaba54 into m4b:master Jan 31, 2021
@m4b
Copy link
Owner

m4b commented Jan 31, 2021

thanks for this PR ! :)

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.

Incorrect file_range for ELF sections with type SHT_NOBITS
2 participants