search

m4b / goblin

An impish, cross-platform binary parsing crate, written in Rust
MIT License
1.17k stars 156 forks source link

pe: offer basic section manipulations #381

Closed RaitoBezarius closed 4 months ago

RaitoBezarius commented 9 months ago

Writing new structures from an existing PE is non-trivial because PE structures may require a complete rewrite of all the structures and recompute of the offsets for space reasons.

We sidestep the difficulty by having a meta Writer structure which tracks the information required to re-render an existing PE information + extra information like new sections.

Depends on #361.

TODO:

RaitoBezarius commented 9 months ago

Whew! The section adder works!

RaitoBezarius commented 6 months ago

OK, it's in a not so-bad state, I can boot PE binaries manipulated by this code adding a bunch of sections.

RaitoBezarius commented 6 months ago

I will move further sanity checking and ordering to another PR if that's possible. I think it's much easier to take this for a walk to discover ideas on how to test it rather than devise testing here. (I found a few bugs by just using it in https://github.com/nix-community/lanzaboote and using our UEFI integration testing).

I can confirm this PR makes all our integration testing pass, this means that the binaries assembled with this section manipulator are quite good.

My plan for follow up are:

After that, anyone who is kind of interested can come and bring COFF support and we will have capabilities to write compilers :).

Unrelated but I have plan to do maybe an ELF writer for a rewrite of https://github.com/NixOS/patchelf in Rust.

@m4b How do you see this PR? Could you give me what you would like to see to be addressed/directions on how to get this merged? Thank you so much for your help!

m4b commented 6 months ago

@m4b How do you see this PR? Could you give me what you would like to see to be addressed/directions on how to get this merged? Thank you so much for your help!

I haven't looked in depth yet, in general it looks fine though.

However, since we just did a release:

  1. I would maybe plan what you envision getting merged for a theoretical 0.9
  2. on that topic, I would try to minimize (or completely remove) breaking changes; I understand some of this might not be possible, but if it isn't, I would really encourage changes made to be future proof as much as possible/include as much as you think you'll need (but not more :) )

I would really like to release your changes as a minor patch, if possible, to increase release cadence. But again, if you have breaking changes that must go in/have a good compelling design reason for, that's ok too, just try to batch these changes and plan subsequent PRs soon enough so that all breaking changes go into 0.9

The back context of this is I think i would like to release 1.0 (i've been saying this for years, I know), but i think it is about time; so with that in mind, perhaps try to design your PRs and changes so that if you have to add something/change something later, it will not be a breaking change.

Lastly, and i have not checked whether it's true, but your writer implementation may better be served as it's own crate, if it doesn't depend on any internal/private apis. That would allow you to release a change with whatever cadence and breaking schedule you like, similar to e.g., faerie.

For similar reasons, if the Pwrite changes were done well, it should ideally be possible to only use goblin's public apis to write a PE writer/object writer crate, and it shouldn't be a requirement for the writer to live in goblin (though i'm not opposed to this either).

Anyway, that's a lot of text, but to summarize:

  1. try to plan your PRs with a 1.0 release in mind, and all that entails
  2. batch breaking changes for a 0.9 release, and let me know how many PRs you envision will be in the 0.9 release
  3. minimize if possible, breaking changes, unless you need them
  4. have fun :)
RaitoBezarius commented 6 months ago

Closing in favor of #389.