Consider stopping use of extra_traits feature due to soundness issues?

[erickt]

There are apparently some soundness issues with libc's extra_traits feature in rust-lang/libc#2700, rust-lang/libc#2064, and rust-lang/libc#2816, with an effort to remove the extra_traits feature to avoid this issue in rust-lang/libc#3692. Would it be worthwhile removing it?

[SteveLauC]

The first linked issue has been resolved on the Nix side, not sure if Nix is affected by the other 2 issues.

From my understanding, removing those derive impl would be a huge breaking change, blindly removing them is not feasible. Let me take a closer look at this.

[tgross35]

Hey Nix team, we will probably have a discussion about this at some point. I'm not sure I fully understand why this would mean a lot of breakage for Nix, at least anything that is worse than the rest of the ecosystem. Would you mind posting a comment here? rust-lang/libc#3880

I think we kind of have to do something here since the current design definitely encourages UB, but maybe we could help mitigate things somehow. I'm not sure what that would mean, open to suggestions.

For what it's worth I don't think we will actually be close to 1.0 until toward the end of the year.

[SteveLauC]

Hi @tgross35, thanks for your work on the libc crate and reaching out to Nix:)

I'm not sure I fully understand why this would mean a lot of breakage for Nix

Nix uses these traits for the derived trait impl of Nix types, if they are removed, Nix has to:

• Removes them as well, or
• Try to manually implement them safely

Removing these things is indeed a breaking change for the libc crate, and if Nix removes them as well, then it is also a breaking change for Nix, I guess?

I think we kind of have to do something here since the current design definitely encourages UB

Yeah, this is understandable.

For what it's worth I don't think we will actually be close to 1.0 until toward the end of the year.

Thanks for letting me know:)

[tgross35]

Appreciate the quick response!

Nix uses these traits for the derived trait impl of Nix types, if they are removed, Nix has to:

* Removes them as well, or

* Try to manually implement them safely

Removing these things is indeed a breaking change for the libc crate, and if Nix removes them as well, then it is also a breaking change for Nix, I guess?

Do you mean source code authored by Nix maintainers, or something like this is available by default for any crate that wants to get packaged on Nix? I could imagine the fallout being pretty different.

I am somewhat torn on this. One one hand it seems to make sense that if everyone is going to have to manually implement PartialEq themselves, then maybe we should just reduce the effort and keep a best-effort implementation in libc. On the other hand, these are just bad implementations; checking pointer equality is unlikely to be useful and sometimes the fields that should be checked are different , which means that each application probably does have a slightly different meaning of equality.

[SteveLauC]

Do you mean source code authored by Nix maintainers, or something like this is available by default for any crate that wants to get packaged on Nix? I could imagine the fallout being pretty different.

Removing these things is indeed a breaking change for the libc crate,

This is for "source code authored by Nix maintainers".

and if Nix removes them as well, then it is also a breaking change for Nix, I guess?

This is for Nix consumers, i.e., the crates that are using Nix. Though this depends on how Nix handle the breaking changes introduced by libc, if Nix "Try to manually implement them safely", then this won't happen.

---

One one hand it seems to make sense that if everyone is going to have to manually implement PartialEq themselves, then maybe we should just reduce the effort and keep a best-effort implementation in libc.

If the impl would make sense to most users, we should do it in the libc crate. And, a user can always opt-out if the libc impl does not make sense.

I am somewhat torn on this.

which means that each application probably does have a slightly different meaning of equality.

Yeah, I agree. I haven't checked the cases in the related libc issues, let me give them a check so that we can have further discussions.

[asomers]

One one hand it seems to make sense that if everyone is going to have to manually implement PartialEq themselves, then maybe we should just reduce the effort and keep a best-effort implementation in libc.

If the impl would make sense to most users, we should do it in the libc crate. And, a user can always opt-out if the libc impl does not make sense.

IMHO the benefits of automatically implementing PartialEq in libc far outweigh the downsides. Maybe pointer equality isn't always very useful, but libc consumers like Nix can easily override it. The only structs where libc shouldn't implement extra traits are those were it can't be done safely, like unions.

[tgross35]

There are other cases that may not be safe - like structs that have extra fields for future compatibility, or those that have padding fields that libc or the kernel might use for scratch, or others that make use of packing so you wind up with unaligned fields. Plus some incompatibilities where the traits might be safe on one platform but not another (that one is easier to represent, just surprising).

But you should echo those points to the issue in rust-lang/libc that I linked so the others see them. Also I don't think we have a very good snapshot of how the feature is actually being used, so if you have some source links those would be helpful to see too.

[asomers]

Extra unused fields might be unhelpful when implementing certain traits, but how could they make it unsafe? And shouldn't the compiler be able to handle unaligned fields?

[tgross35]

(responded at rust-lang/libc#3880)