Closed sylvestre closed 7 months ago
I think this has to do with how we generate our pam bindings, tagging @rnijveld
Yeah this is because we stopped "auto-generating" the C bindings.
Some solution directions:
I tried option 1 here https://github.com/memorysafety/sudo-rs/pull/820 but it is failing because the generated bindings contain a bunch of unused items. I seem to remember we were trimming those by hand? Another issue here is that I don't happen to have an ARM linker so I couldn't handle those cases myself and I assume installing those on the CI runner would be annoying as well.
The portable part I think it is harder as we would eventually hit ABI compatibility issues. One easy example would be varargs where x86 represents the vararg list in one way and arm in a different way.
There's always the option of just going back to calling bindgen while building and not keeping the src/pam/sys.rs
file in-tree
That's always a fallback (which we can document clearer, perhaps: if these tests fail, regenerate the bindings using command
, etc), but then the benefits should outweigh the reasons we decided to move away from that.
Or we could use bindgen, but write a checker that checks that the generated bindings only contain extern "C"
things, and in particular only the "minimally required" identifiers; that would probably grant enough build-time security. But I'm guesstimating that such a checker is small project in itself (maybe something fun to do as a downtime project) and just using #1 is better for now.
Another option would be to write non-varargs trampoline functions in portable ISO C and generate bindings for our own C functions, but I guess I would be the only one in favour of that 😄
As for the binding minimisation, we didn't do that by hand, we used https://crates.io/crates/cargo-minify by me and @Tortoaster.
I'll try to get #1 working then. Maybe I'll integrate cargo-minify on it. However I think we would only get i386 support out of it. Maybe we could add this fallback under a feature so people can just generate the PAM bindings on their platforms themselves
Hmmm. If we only get x86 (32-bit) support out of it I'd rather not spend too much time on it (it's not an irrelevant architecture yet perhaps, but it clearly doesn't have a bright future).
Of course the fallback option could be tied to the tests: if the binding tests fail, we can emit a compiler warning with the bindgen command that the developer/packager needs to run to fix the problem.
I'm a bit interested in the "go portable" option still, I'll see if (as an experiment) I can make something plausible there in a reasonably short time (do we really use many vararg functions, for example?)
Edit: we don't actually use varargs at all, it gets generated (and not minified) because it is a pub
type. But I can remove that code (and the accompanying test code) manually.
Further comment: most of the tests are failing because they hardcode the size of the types; I'm not sure if this is really necessary (the Rust code doesn't do any funny business on them), but in any case we can handcode those to be flexible.
Approach #2 (generate portable bindings) worked for me: tested it on a cargo --target=i686-unknown-linux-gnu
build. See PR #822.
We should also seriously consider removing the bindgen tests; the PAM bindings have by now been basically hand-checked.
Would it be possible to have a new release (for this patch in particular) ? thanks :)
Thanks for asking.
Yes, it seems like a good idea to do a minor update release soon, indeed. Also given some other bug-fixes we have done since 0.2.1. I think we can do one within two weeks.
I guess it is a 32 bit issue
builds logs: https://ci.debian.net/packages/r/rust-sudo-rs/testing/armel/41804299/ https://ci.debian.net/packages/r/rust-sudo-rs/testing/armhf/41804473/ https://ci.debian.net/packages/r/rust-sudo-rs/testing/i386/41804658/
An example: