Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SigSet: Fix incorrect implementation of Eq, PartialEq and Hash #1946

Merged
merged 1 commit into from
Sep 30, 2023

Conversation

germag
Copy link
Contributor

@germag germag commented Dec 19, 2022

signal::SigSet derives Eq, PartialEq and Hash, but on linux, sigfillset(3) only initializes the first 64 bits, leaving the rest uninitialized. So two signal::SigSets can be different even though both have the same set of signals.

closes #1749

@germag
Copy link
Contributor Author

germag commented Dec 19, 2022

v2:

  • Fix formatting issue

@rtzoeller
Copy link
Collaborator

@germag have you done any performance analysis of the auto-derived version (on platforms other than Linux; I acknowledge the Linux impl is broken) vs the manually implemented one?

If this new implementation is measurably slower, we might want to use the auto-derived version on platforms where it is correct.

We could also implement this on Linux by reading only the initialized bits and hashing them all at once, although that is certainly a leaky abstraction.

@germag
Copy link
Contributor Author

germag commented Dec 20, 2022

@germag have you done any performance analysis of the auto-derived version (on platforms other than Linux; I acknowledge the Linux impl is broken) vs the manually implemented one?

If this new implementation is measurably slower, we might want to use the auto-derived version on platforms where it is correct.

No, I haven't, sigismember() it's a simple function, but there will certainly be a difference in performance. However, IIRC according to posix we must treat sigset_t as an opaque object. Anyway, I can implement it only for linux, but I don't know if is also required in other targets.

We could also implement this on Linux by reading only the initialized bits and hashing them all at once, although that is certainly a leaky abstraction.

I think it would be better to do that in libc. Also, the __val[] field inside sigset_t is not public.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should merge this. It's hard for Nix to know which platforms fully initialize sigset_t and which don't. So unless they start providing sigeqsetor something like that, we should probably just do things the hard way.

@SteveLauC
Copy link
Member

Friendly ping @germag, would you like to finish this PR? The CI failure seems to be irrelevant, rebasing your branch should resolve it :)

@germag
Copy link
Contributor Author

germag commented Sep 30, 2023

Rebase

Friendly ping @germag, would you like to finish this PR? The CI failure seems to be irrelevant, rebasing your branch should resolve it :)

Sorry for the delay, I thought it was already merged

CHANGELOG.md Outdated Show resolved Hide resolved
`signal::SigSet` derives `Eq`, `PartialEq` and `Hash`, but on linux, sigfillset(3)
only initializes the first 64 bits, leaving the rest uninitialized.
So two `signal::SigSets` can be different even though both have the
same set of signals.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@SteveLauC SteveLauC added this pull request to the merge queue Sep 30, 2023
Merged via the queue into nix-rust:master with commit 57663c0 Sep 30, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SigSet Incorrect implementation of Eq and PartialEq
4 participants