libc icon indicating copy to clipboard operation
libc copied to clipboard

Invalid PartialEq implementation for unions

Open asomers opened this issue 3 years ago • 7 comments

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.

asomers avatar Jun 03 '22 17:06 asomers

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.

RalfJung avatar Jun 03 '22 18:06 RalfJung

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?

asomers avatar Jun 03 '22 19:06 asomers

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.

RalfJung avatar Jun 03 '22 20:06 RalfJung

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.

notgull avatar Jun 04 '22 16:06 notgull

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

asomers avatar Jun 04 '22 16:06 asomers

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

Ralith avatar Apr 05 '24 21:04 Ralith

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

tgross35 avatar Aug 29 '24 06:08 tgross35