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

Change SigAction::flags to use from_bits_truncated #869

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

Detegr
Copy link
Contributor

@Detegr Detegr commented Mar 2, 2018

On Linux, if the signal trampoline code is in the C library, sigaction
sets the SA_RESTORER flag (0x04000000) in the sa_flags field of old
sigaction (see sigreturn(2)).

This is not intended for application use and is missing from SaFlags,
therefore from_bits fails and unwrapping panics the user program.

This fix just drops the bits that are not defined in SaFlags.

@asomers
Copy link
Member

asomers commented Mar 2, 2018

Do you have an example testcase that demonstrates the failure?

@Detegr
Copy link
Contributor Author

Detegr commented Mar 2, 2018

Sure.

extern crate nix;
use nix::sys::signal;

extern "C" fn handler(_: nix::libc::c_int) {}
fn main() {
    let act = signal::SigAction::new(
        signal::SigHandler::Handler(handler),
        signal::SaFlags::empty(),
        signal::SigSet::empty(),
    );
    let oact = unsafe { signal::sigaction(signal::SIGINT, &act) }.unwrap();
    println!("{:?}", oact.flags()); // Works, the default handler flags do not have SA_RESTORER set
    let oact = unsafe { signal::sigaction(signal::SIGINT, &act) }.unwrap();
    println!("{:?}", oact.flags()); // Panics, as sa_flags contains SA_RESTORER
}

@asomers
Copy link
Member

asomers commented Mar 2, 2018

Yeah, that fails for me too, on Linux but not FreeBSD. Could you please add it to tests/sys/test_signal.rs?

@Detegr
Copy link
Contributor Author

Detegr commented Mar 2, 2018

Added.

On Linux, if the signal trampoline code is in the C library, sigaction
sets the SA_RESTORER flag (0x04000000) in the sa_flags field of old
sigaction (see sigreturn(2)).

This is not intended for application use and is missing from SaFlags,
therefore from_bits fails and unwrapping panics the user program.

This fix just drops the bits that are not defined in SaFlags.
@asomers
Copy link
Member

asomers commented Mar 2, 2018

bors r+

bors bot added a commit that referenced this pull request Mar 2, 2018
869: Change SigAction::flags to use from_bits_truncated r=asomers a=Detegr

On Linux, if the signal trampoline code is in the C library, sigaction
sets the SA_RESTORER flag (0x04000000) in the sa_flags field of old
sigaction (see sigreturn(2)).

This is not intended for application use and is missing from SaFlags,
therefore from_bits fails and unwrapping panics the user program.

This fix just drops the bits that are not defined in SaFlags.
@bors
Copy link
Contributor

bors bot commented Mar 2, 2018

@bors bors bot merged commit 4419205 into nix-rust:master Mar 2, 2018
@Susurrus
Copy link
Contributor

Susurrus commented Apr 3, 2018

This needed a CHANGELOG entry indicating the fix as well. @Detegr Would you be willing to submit one?

Detegr added a commit to Detegr/nix that referenced this pull request Apr 4, 2018
@Detegr
Copy link
Contributor Author

Detegr commented Apr 4, 2018

Sure. Is it okay to have the commit in this PR or should I open a new one?

@Susurrus
Copy link
Contributor

Susurrus commented Apr 4, 2018

Since this was merged, you should open a new one. As all you're changing is the CHANGELOG, put "[skip ci]" at the start of the commit message. Thanks!

Detegr added a commit to Detegr/nix that referenced this pull request Apr 4, 2018
@Detegr Detegr mentioned this pull request Apr 4, 2018
Detegr added a commit to Detegr/nix that referenced this pull request May 1, 2018
bors bot added a commit that referenced this pull request May 2, 2018
880: Update changelog for #869 r=Susurrus a=Detegr

Updated changelog to contain #869

Co-authored-by: Antti Keränen <detegr@gmail.com>
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