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

Mark __getrandom_custom unsafe #341

Closed
wants to merge 1 commit into from
Closed

Mark __getrandom_custom unsafe #341

wants to merge 1 commit into from

Conversation

LegionMammal978
Copy link

Right now, the lack of documentation is the only thing stopping callers from directly calling __getrandom_custom() from safe code:

#![forbid(unsafe_code)]

use getrandom::register_custom_getrandom;
use std::ptr;

fn example(buf: &mut [u8]) -> Result<(), getrandom::Error> {
    buf[0] = 0;
    Ok(())
}

register_custom_getrandom!(example);

fn main() {
    __getrandom_custom(ptr::null_mut(), 1); // segfault
}

Marking the function unsafe should discourage this.

(Also, note that it is currently formally considered UB if an extern "C" function unwinds after a panic, and there is nothing in __getrandom_custom() to prevent the callback from unwinding through it. However, the Rust project has plans to automatically add an unwind-to-abort shim to extern "C" functions (rust-lang/rust#74990), so waiting for that may be simpler than adding catch_unwind() or a drop guard.)

@newpavlov
Copy link
Member

Looks good. __getrandom_custom is semantically a private item (like __Nonexhaustive was before #[non_exhaustive] was introduced), but you are right that it's probably better to mark it unsafe.

Also, note that it is currently formally considered UB if an extern "C" function unwinds after a panic, and there is nothing in __getrandom_custom() to prevent the callback from unwinding through it

Hm, we may want to use extern "Rust" because of that. We discussed potentially using it when this feature was introduced, but I don't remember exactly why we decided against it.

josephlr added a commit that referenced this pull request Mar 9, 2023
This supersedes #341, and makes the following changes
  - All the code for implementing `__getrandom_custom` is now in an
    unnamed `const` block (stable since 1.37)
    - I found this approch [here](https://internals.rust-lang.org/t/anonymous-modules/15441)
    - Nothing inside the block can be referenced outside of it
  - `__getrandom_custom` is marked `unsafe`
    - It can't be accessed externally, but is "logically" unsafe as it
      dereferences raw pointers
  - The type of the function is moved to a typedef, so we can check that
    the defined type matches that of `getrandom:getrandom`.
  - Use `::core::result::Result` instead of `Result`
    - Similar to use use of `from_raw_parts_mut` this prevents
      compilation errors if `Result` is redefined.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member

josephlr commented Mar 9, 2023

@newpavlov @LegionMammal978 I opened #344 which overhauls the custom implementation macro and includes these changes. Specifically, the implementation is now inaccessible, so external users cannot use it at all, regardless of if it is unsafe or not.

@newpavlov if #344 looks good to you, approve it and close this PR.

@josephlr
Copy link
Member

josephlr commented Mar 9, 2023

The calling ABI questions are more complicated, I opened #345 to just track the ABI stuff.

josephlr added a commit that referenced this pull request Mar 9, 2023
This supersedes #341, and makes the following changes
  - All the code for implementing `__getrandom_custom` is now in an
    **named** `const` block (unnamed consts require Rust 1.37)
    - I found this approch [here](https://internals.rust-lang.org/t/anonymous-modules/15441)
    - Nothing inside the block can be referenced outside of it
  - `__getrandom_custom` is marked `unsafe`
    - It can't be accessed externally, but is "logically" unsafe as it
      dereferences raw pointers
  - The type of the function is moved to a typedef, so we can check that
    the defined type matches that of `getrandom:getrandom`.
  - Use `::core::result::Result` instead of `Result`
    - Similar to use use of `from_raw_parts_mut` this prevents
      compilation errors if `Result` is redefined.

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit that referenced this pull request Mar 9, 2023
This supersedes #341, and makes the following changes
  - All the code for implementing `__getrandom_custom` is now in an
    **named** `const` block (unnamed consts require Rust 1.37)
    - I found this approch [here](https://internals.rust-lang.org/t/anonymous-modules/15441)
    - Nothing inside the block can be referenced outside of it
  - `__getrandom_custom` is marked `unsafe`
    - It can't be accessed externally, but is "logically" unsafe as it
      dereferences raw pointers
  - The type of the function is moved to a typedef, so we can check that
    the defined type matches that of `getrandom:getrandom`.
  - Use `::core::result::Result` instead of `Result`
    - Similar to use use of `from_raw_parts_mut` this prevents
      compilation errors if `Result` is redefined.

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit that referenced this pull request Mar 10, 2023
This supersedes #341, and makes the following changes
  - All the code for implementing `__getrandom_custom` is now in an
    **named** `const` block (unnamed consts require Rust 1.37)
    - I found this approch [here](https://internals.rust-lang.org/t/anonymous-modules/15441)
    - Nothing inside the block can be referenced outside of it
  - `__getrandom_custom` is marked `unsafe`
    - It can't be accessed externally, but is "logically" unsafe as it
      dereferences raw pointers
  - The type of the function is moved to a typedef, so we can check that
    the defined type matches that of `getrandom:getrandom`.
  - Use `::core::result::Result` instead of `Result`
    - Similar to use use of `from_raw_parts_mut` this prevents
      compilation errors if `Result` is redefined.

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit that referenced this pull request Mar 10, 2023
This supersedes #341, and makes the following changes
  - All the code for implementing `__getrandom_custom` is now in an
    **named** `const` block (unnamed consts require Rust 1.37)
    - I found this approch [here](https://internals.rust-lang.org/t/anonymous-modules/15441)
    - Nothing inside the block can be referenced outside of it
  - `__getrandom_custom` is marked `unsafe`
    - It can't be accessed externally, but is "logically" unsafe as it
      dereferences raw pointers
  - The type of the function is moved to a typedef, so we can check that
    the defined type matches that of `getrandom:getrandom`.
  - Use `::core::result::Result` instead of `Result`
    - Similar to use use of `from_raw_parts_mut` this prevents
      compilation errors if `Result` is redefined.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member

Closing in favor of #344

@josephlr josephlr closed this Mar 10, 2023
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.

None yet

3 participants