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: A new unsafe helper method to create a SigSet from a sigset_t #1741

Merged
merged 2 commits into from
Jul 15, 2022

Conversation

germag
Copy link
Contributor

@germag germag commented Jun 8, 2022

Currently, the only way to create a SigSet from a sigset_t object
is by using pointer casts, like:

unsafe {
    let sigset = *(&sigset as *const libc::sigset_t as *const SigSet)
};

This is un-ergonomic for library creators with interfaces to C.
So, let's add a new unsafe method that creates a SigSet from a
libc::sigset_t object.

We can't implement From since converting from libc::sigset_t to
SigSet is unsafe, because objects of type libc::sigset_t must be
initialized by calling either sigemptyset(3) or sigfillset(3)
before being used. In other case, the results are undefined.
We can't implement TryFrom either, because there is no way to check
if an object of type libc::sigset_t is initialized.

Signed-off-by: German Maglione gmaglione@redhat.com

CHANGELOG.md Outdated Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
@germag
Copy link
Contributor Author

germag commented Jun 9, 2022

v2:

  • Fix links and ticks in comments
  • Add link to PR in Changelog
  • Fix typo and ticks in commit message

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.

The FreeBSD test failure should be fixed by rebasing.

src/sys/signal.rs Show resolved Hide resolved
@germag
Copy link
Contributor Author

germag commented Jun 10, 2022

v3:

  • Add # Sefety section
  • Rebase

@germag germag requested a review from asomers June 17, 2022 16:41
@germag
Copy link
Contributor Author

germag commented Jun 20, 2022

v4:

  • Add repr(transparent)attribute to SigSet struct

@germag
Copy link
Contributor Author

germag commented Jun 20, 2022

I know it's unrelated to this PR, but SigSet derives Eq and PartialEq, I assume because libc::sigset_t does it too, but at least in linux, sigfillset(3) leaves bits uninitialized (IIRC it only initializes the first 64 bits), so two SigSets can be different even though they have the same set of signals

@asomers
Copy link
Member

asomers commented Jun 20, 2022

I know it's unrelated to this PR, but SigSet derives Eq and PartialEq, I assume because libc::sigset_t does it too, but at least in linux, sigfillset(3) leaves bits uninitialized (IIRC it only initializes the first 64 bits), so two SigSets can be different even though they have the same set of signals

Ooh, good catch. Could you open an issue for this?

@germag
Copy link
Contributor Author

germag commented Jun 21, 2022

Ooh, good catch. Could you open an issue for this?

Done

@germag
Copy link
Contributor Author

germag commented Jun 21, 2022

Rebase

@germag
Copy link
Contributor Author

germag commented Jun 28, 2022

it has been suggested to me that new_unchecked() may be a better name than from_sigset_t_unchecked(), WDYT?

@germag germag requested a review from rtzoeller July 7, 2022 10:47
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Jul 8, 2022
1741: SigSet: A new unsafe helper method to create a SigSet from a sigset_t r=rtzoeller a=germag

Currently,  the only way to create a `SigSet` from a `sigset_t` object
is by using pointer casts, like:

```
unsafe {
    let sigset = *(&sigset as *const libc::sigset_t as *const SigSet)
};
```

This is un-ergonomic for library creators with interfaces to C.
So, let's add a new unsafe method that creates a `SigSet` from a 
`libc::sigset_t` object.

We can't implement `From` since converting from `libc::sigset_t` to
`SigSet` is unsafe, because objects of type `libc::sigset_t` must be
initialized by calling either `sigemptyset(3)` or `sigfillset(3)`
before being used. In other case, the results are undefined.
We can't implement `TryFrom` either, because there is no way to check
if an object of type `libc::sigset_t` is initialized.

Signed-off-by: German Maglione <gmaglione@redhat.com>

Co-authored-by: German Maglione <gmaglione@redhat.com>
@bors
Copy link
Contributor

bors bot commented Jul 8, 2022

Build failed:

@rtzoeller
Copy link
Collaborator

@germag can you update the commit adding the test to fix the formatting issue, and also fix the typos in the changelog (e.g. in transparent), since we're updating things anyways?

Currently,  the only way to create a `SigSet` from a `sigset_t` object
is by using pointer casts, like:

```
unsafe {
    let sigset = *(&sigset as *const libc::sigset_t as *const SigSet)
};
```

This is un-ergonomic for library creators with interfaces to C.
So, let's add a new unsafe method that creates a `SigSet` from a
`libc::sigset_t` object.

We can't implement `From` since converting from `libc::sigset_t` to
`SigSet` is unsafe, because objects of type `libc::sigset_t` must be
initialized by calling either `sigemptyset(3)` or `sigfillset(3)`
before being used. In other case, the results are undefined.
We can't implement `TryFrom` either, because there is no way to check
if an object of type `libc::sigset_t` is initialized.

Signed-off-by: German Maglione <gmaglione@redhat.com>
This commit adds the `repr(transparent)` attribute to the `SigSet`
struct, to make sure that its representation is exactly like the
`sigset_t` struct from C, in all cases.

Signed-off-by: German Maglione <gmaglione@redhat.com>
@germag
Copy link
Contributor Author

germag commented Jul 12, 2022

v5:

  • Remove empty line
  • Fix Changelog typos
  • Rebase

@germag
Copy link
Contributor Author

germag commented Jul 12, 2022

@germag can you update the commit adding the test to fix the formatting issue, and also fix the typos in the changelog (e.g. in transparent), since we're updating things anyways?

Done, thanks

Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit b44daa1 into nix-rust:master Jul 15, 2022
@germag germag deleted the feat-unsafe-from branch July 15, 2022 07:33
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.

3 participants