patchelf icon indicating copy to clipboard operation
patchelf copied to clipboard

Improve testability

Open brenoguim opened this issue 2 years ago • 6 comments

Hi! I would like to suggest (and implement if agreed) an improvement to the testability of the project.

I noticed that it's hard to exercise a specific part of the tool which I'll call the Layout Engine. To me, it's everything in rewriteSectionsExecutable and rewriteSectionLibrary except for the writeReplacedSections and writeHeaders.

The current approach to testing is to put whole ELF files and run through the whole flow - reading the elf file, understanding its contents, looking at .dynamic, .dynstr and so on, then doing layout and finally writing.

But the Layout Engine, is largely independent of the content of the elf files. All this engine needs is:

  • What are the sections offsets/addr/size?
  • What are the segment offsets/vaddr/paddr
  • Restrictions (content that can't be moved in the address space, sections that must come first...)
  • What layout strategy you want: Put sections in the end, or put sections in the beginning
  • What section you want to grow/shrink

So, my suggestion (and desire to implement) is a separate class that takes these inputs, calculates the new layout and spits it out.

Then the caller will use this result to write the appropriate data, adjust special offsets and the more elf specific stuff.

This will improve the testing in the following ways:

  1. Human readable test: We will be able to store text layout files that are easy to construct by hand. These layout files would be used to test the LayoutEngine in isolation and verify properties about its results. For example, sections in the file should never intersect (like it happened for a bug I investigated).
  2. Encourage more tests: We will be able to request users to --generate-layout-dump if they can't share the whole binary. They would be able to read the text file and see there is nothing confidential there and would give us something to work with. This would make it much easier to add tests from bugs.
  3. Fuzz testing - Allow us to easily generate random scenarios to make sure the engine never crashes or spits something with broken invariants (like overlapping sections).

I would love to implement something like that, and I'm confident that I can do it in a way that you are comfortable receiving it (that means no big rewrites, focus on small incremental changes, tests...).

But before I write any of that, I would like to hear if this makes sense for the project, if someone is already working on it, or if it's too out there.

Thanks!

brenoguim avatar Feb 23 '23 23:02 brenoguim

I like the idea. Thumbs up from my side.

Mic92 avatar Feb 24 '23 09:02 Mic92

cc @cgzones

Mic92 avatar Feb 24 '23 09:02 Mic92

I'm confident that I can do it in a way that you are comfortable receiving it (that means no big rewrites, focus on small incremental changes, tests...).

I wrote the whole thing twice and I still failed at this part :) Maybe a better idea is to have infrastructure to auto-generate true ELF files from a high level description.

brenoguim avatar Mar 06 '23 23:03 brenoguim

Sorry, I have been a bit radio silent the last two weeks. I have just accumulated different projects over the years and now I am cycling through them for reviews. Would you be open to also be added as a maintainer so we can share the review load? The only constraint that this project has is that needs to work for the usecases in nixpkgs and that we can easily bootstrap it from a small set of dependencies (i.e. c++ compiler + coreutils + linker).

Mic92 avatar Mar 07 '23 11:03 Mic92

Hey @Mic92 , I would love to participate as a maintainer! The constraints are fair and pretty much what I would expect from patchelf anyways. I think my contributions goin forward will be on adding more tests (so that later one day we can confidently take bigger changes), review PRs and address crashes and urgent bugs.

brenoguim avatar Mar 07 '23 17:03 brenoguim

Welcome to the team. You should have received an invite.

Mic92 avatar Mar 08 '23 08:03 Mic92