rust-lang / libc

Raw bindings to platform APIs for Rust
https://docs.rs/libc
Apache License 2.0
2.06k stars 1.03k forks source link

Invalid PartialEq implementation for unions #2816

Open asomers opened 2 years ago

asomers commented 2 years ago

union semun, in unix/bsd/apple/mod.rs, has three fields. On LP64, these fields will have unequal sizes. But its PartialEq and Hash implementations only check the smallest field. So the following code should fail (untested, because I don't have access to a Mac):

use libc::semun;

#[test]
fn semun_eq() {
    let x = semun{ val: 0x44556677 };
    let y = semun{ array: 0x0011223344556677 };
    assert!(x != y);
}

A similar situation holds on Linux for __c_anonymous_ptrace_syscall_info_data. That union has three members of different sizes, and its PartialEq implementation will return turn as long as any pair of fields compare equal. Effectively, that means it's only comparing the smallest field.

There are other unions in libc, but I haven't audited them all.

RalfJung commented 2 years ago

Note that an implementation that compares the larger, ptr-sized fields would be unsound, since semun { val: 0 } leaves those fields uninitialized -- so reading them (at integer or raw ptr type) is UB.

asomers commented 2 years ago

Crap, I never thought of that. Are you saying that it's impossible to correctly implement PartialEq for a union type then? In that case, would the only way to do it be to create a newtype that always zero-initializes the entire union before initializing any field?

RalfJung commented 2 years ago

Are you saying that it's impossible to correctly implement PartialEq for a union type then?

Depends on what you mean by 'correctly'... if you mean it should compare all the bytes, then yes.

Probably the type should just not implement PartialEq since the expected semantics cannot be provided. But it might be too late for that due to backwards compatibility constraints.

notgull commented 2 years ago

But it might be too late for that due to backwards compatibility constraints.

Is there some way to indicate that the PartialEq implementation is deprecated? I think that would be useful if we wanted to move in the direction of removing it.

asomers commented 2 years ago

I don't think so. AFAIK you still can't deprecate a trait impl https://github.com/rust-lang/rust/issues/39935 .

Ralith commented 5 months ago

New instance of this unsoundness are still being introduced, e.g.:

https://github.com/rust-lang/libc/blob/a0f5b4b21391252fe38b2df9310dc65e37b07d9f/src/unix/haiku/native.rs#L516-L525

tgross35 commented 2 weeks ago

To be discussed as part of https://github.com/rust-lang/libc/issues/3880