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

Sanitize ELF Sections API #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Keno
Copy link
Member

@Keno Keno commented Nov 27, 2020

ELF Sections are kinda 1-indexed in that while there is a Section 0,
it is always of type SHT_NULL and is otherwise invalid to reference.
Further, later ELF extensions have re-used these unused bits for
extensions, notably for ELF files with more than 0xff00 sections.
This patch does the following:

  • Prevents section 0 from being returned by "Sections", since it isn't
    actually a real section and treating it as such would be incorrect
    if the section header stores extension data (before it just doesn't
    make a difference)
  • As a result indices into Sections now actually match the ELF section
    number rather than being off by one.
  • For good measure, implement the ELF large sections extension and add
    a test to make sure we can handle a file with as many sections.

ELF Sections are kinda 1-indexed in that while there is a Section 0,
it is always of type SHT_NULL and is otherwise invalid to reference.
Further, later ELF extensions have re-used these unused bits for
extensions, notably for ELF files with more than 0xff00 sections.
This patch does the following:

- Prevents section 0 from being returned by "Sections", since it isn't
  actually a real section and treating it as such would be incorrect
  if the section header stores extension data (before it just doesn't
  make a difference)
- As a result indices into `Sections` now actually match the ELF section
  number rather than being off by one.
- For good measure, implement the ELF large sections extension and add
  a test to make sure we can handle a file with as many sections.
@Keno Keno requested a review from staticfloat November 27, 2020 04:30
Copy link
Collaborator

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

Looks like tests are failing.

src/ELF/ELFHandle.jl Outdated Show resolved Hide resolved
for i = 0:0xffff
print(f, ".section .text$i\nfoo$i:\nret\n")
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this file actually used for anything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's supposed to be used to generate largesectcount.S but it seems to be outputting to test.S instead.
Would be nice if it compiled it to largesectcount.o as well.

@staticfloat
Copy link
Collaborator

As an aside, if possible, I'd like it if we can avoid embedding the large test files into the git repository, as they'll actually significantly inflate user install sizes. We can either store only the generator .jl file and rely on a local compiler to run the tests here, or we can store them in a lazy artifact and download them only for the tests, implicitly restricting the compatibility of this package to 1.3+.

@staticfloat
Copy link
Collaborator

Can you rebase on latest master so you can pick up the new GHA config?

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