netbsd: Simplify weak lookup. #484

briansmith · 2024-06-18T20:27:25Z

Remove LazyPtr. Avoid constructing an invalid pointer as a sentinel to indicate that the pointer is uninitialized. Now, a null pointer means it is uninitialized, and a non-null pointer means it is initialized. This is less questionable from a safety perspective, and should also be more efficient.

Reduce duplication between the "getrandom is available" and the fallback case.

newpavlov · 2024-06-18T20:58:19Z

What about the following comment about KERN_ARND?

NetBSD will only return up to 256 bytes at a time, and older NetBSD kernels will fail on longer buffers.

Is it no longer relevant? To be safe it may be worth to use let buflen = min(buflen, 256) in the polyfill function.

src/netbsd.rs

briansmith · 2024-06-18T22:33:29Z

What about the following comment about KERN_ARND?

NetBSD will only return up to 256 bytes at a time, and older NetBSD kernels will fail on longer buffers.

Is it no longer relevant? To be safe it may be worth to use let buflen = min(buflen, 256) in the polyfill function.

Thanks. As I mentioned in the other comment, I did make this change.

newpavlov

Outside of several minor nits, LGTM.

src/netbsd.rs

newpavlov · 2024-06-18T23:10:25Z

src/netbsd.rs

+    let ptr = match unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) } {
+        ptr if !ptr.is_null() => ptr,
+        _ => polyfill_using_kern_arand as *mut libc::c_void,
+    };


I think the following code will be a bit easier to read:

let mut ptr = unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) }; // If getrandom(2) is not availalbe, use KERN_ARAND polyfill if ptr.is_null() { ptr = polyfill_using_kern_arand as *mut libc::c_void; }

In other PRs, @josephlr asked me to write things the way I wrote it here, and to not write it the let mut ... way.

BTW, the other reason I use a match is because it helps with code coverage measurement. Though we don't have that yet, I hope we do soon.

Ok. :) I don't have a strong preference here, so I will wait for him to confirm that he prefers this way.

it helps with code coverage measurement

I think the if branch should be handled by code coverage tools without issues.

In other PRs, @josephlr asked me to write things the way I wrote it here, and to not write it the let mut ... way.

Looking at this, I think that was a bad call on my part (sorry). I agree w/ @newpavlov that the if is more readable here. @briansmith's point is a good one about code coverage, but we would ideally handle that via branch coverage (see #288 (comment))

src/netbsd.rs

josephlr

Looks nice! Thanks for simplifying this.

josephlr · 2024-06-19T01:30:55Z

src/netbsd.rs

+    let ptr = match unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) } {
+        ptr if !ptr.is_null() => ptr,
+        _ => polyfill_using_kern_arand as *mut libc::c_void,
+    };


In other PRs, @josephlr asked me to write things the way I wrote it here, and to not write it the let mut ... way.

Looking at this, I think that was a bad call on my part (sorry). I agree w/ @newpavlov that the if is more readable here. @briansmith's point is a good one about code coverage, but we would ideally handle that via branch coverage (see #288 (comment))

josephlr · 2024-06-19T01:33:59Z

src/netbsd.rs

@@ -24,28 +44,33 @@ fn kern_arnd(buf: &mut [MaybeUninit<u8>]) -> libc::ssize_t {

 type GetRandomFn = unsafe extern "C" fn(*mut u8, libc::size_t, libc::c_uint) -> libc::ssize_t;


Should the first argument be *mut c_void here? That's what the man pages has: https://man.freebsd.org/cgi/man.cgi?query=getrandom&sektion=2

I'm not sure if it particularly matters

I'm not sure if it particularly matters

In theory it could matter depending on the CFI rules. I changed it.

src/netbsd.rs

Match the type signature that the NetBSD man page gives for getrandom.

Remove LazyPtr. Avoid constructing an invalid pointer as a sentinel to indicate that the pointer is uninitialized. Now, a null pointer means it is uninitialized, and a non-null pointer means it is initialized. This is less questionable from a safety perspective, and should also be more efficient. Reduce duplication between the "getrandom is available" and the fallback case.

briansmith · 2024-06-19T03:33:37Z

src/netbsd.rs

+    if ptr.is_null() {
+        // Verify `polyfill_using_kern_arand` has the right signature.
+        const POLYFILL: GetRandomFn = polyfill_using_kern_arand;
+        ptr = POLYFILL as *mut c_void;


I tweaked this by splitting the conversion into two clearly-correct parts because polyfill_using_kern_arand as *mut c_void is quite a dangerous cast.

briansmith · 2024-06-19T03:35:25Z

src/netbsd.rs

@@ -22,30 +42,37 @@ fn kern_arnd(buf: &mut [MaybeUninit<u8>]) -> libc::ssize_t {
    }
 }

-type GetRandomFn = unsafe extern "C" fn(*mut u8, libc::size_t, libc::c_uint) -> libc::ssize_t;
+type GetRandomFn = unsafe extern "C" fn(*mut c_void, libc::size_t, libc::c_uint) -> libc::ssize_t;


I changed this signature as @josephlr suggested.

briansmith · 2024-06-19T03:35:48Z

src/netbsd.rs

+    // the one in libstd, meaning that the use of non-Relaxed operations is
+    // probably unnecessary.
+    let mut fptr = GETRANDOM.load(Ordering::Acquire);
+    if fptr.is_null() {


Changed this from the match style that people didn't like.

briansmith · 2024-06-19T03:36:18Z

src/netbsd.rs

    static NAME: &[u8] = b"getrandom\0";
    let name_ptr = NAME.as_ptr().cast::<libc::c_char>();
-    unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) }
+    let mut ptr = unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) };
+    if ptr.is_null() {


Changed this from the match style that people didn't like.

briansmith force-pushed the b/netbsd-weak branch 6 times, most recently from c008fee to 1a0a475 Compare June 18, 2024 20:48

briansmith force-pushed the b/netbsd-weak branch 2 times, most recently from e8095c6 to 5d152be Compare June 18, 2024 21:02

newpavlov reviewed Jun 18, 2024

View reviewed changes

src/netbsd.rs Outdated Show resolved Hide resolved

briansmith force-pushed the b/netbsd-weak branch 5 times, most recently from 40669fd to ea3436e Compare June 18, 2024 21:41

briansmith marked this pull request as ready for review June 18, 2024 22:25

briansmith force-pushed the b/netbsd-weak branch from ea3436e to bbf8763 Compare June 18, 2024 22:34

briansmith mentioned this pull request Jun 18, 2024

use_file: Just use libstd on targets that we know have libstd, instead of pthreads + libc. #453

Closed

newpavlov reviewed Jun 18, 2024

View reviewed changes

briansmith force-pushed the b/netbsd-weak branch from bbf8763 to f6bfcdd Compare June 18, 2024 23:34

josephlr approved these changes Jun 19, 2024

View reviewed changes

briansmith added 2 commits June 18, 2024 20:31

netbsd: Use c_void for GetRandomFn first argument type.

8f05eec

Match the type signature that the NetBSD man page gives for getrandom.

briansmith force-pushed the b/netbsd-weak branch from f6bfcdd to 9bf6192 Compare June 19, 2024 03:32

briansmith commented Jun 19, 2024

View reviewed changes

josephlr merged commit 267639e into rust-random:master Jun 19, 2024
54 checks passed

briansmith mentioned this pull request Jun 19, 2024

Implementation of dlsym-based Weak is not sandbox-friendly #428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netbsd: Simplify weak lookup. #484

netbsd: Simplify weak lookup. #484

briansmith commented Jun 18, 2024

newpavlov commented Jun 18, 2024 •

edited

Loading

briansmith commented Jun 18, 2024

newpavlov left a comment

newpavlov Jun 18, 2024

briansmith Jun 18, 2024

briansmith Jun 18, 2024 •

edited

Loading

newpavlov Jun 18, 2024 •

edited

Loading

josephlr Jun 19, 2024

josephlr left a comment

josephlr Jun 19, 2024

josephlr Jun 19, 2024

briansmith Jun 19, 2024

briansmith Jun 19, 2024

briansmith Jun 19, 2024

briansmith Jun 19, 2024

briansmith Jun 19, 2024

		@@ -24,28 +44,33 @@ fn kern_arnd(buf: &mut [MaybeUninit<u8>]) -> libc::ssize_t {

		type GetRandomFn = unsafe extern "C" fn(*mut u8, libc::size_t, libc::c_uint) -> libc::ssize_t;

netbsd: Simplify weak lookup. #484

netbsd: Simplify weak lookup. #484

Conversation

briansmith commented Jun 18, 2024

newpavlov commented Jun 18, 2024 • edited Loading

briansmith commented Jun 18, 2024

newpavlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

newpavlov Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov commented Jun 18, 2024 •

edited

Loading

briansmith Jun 18, 2024 •

edited

Loading

newpavlov Jun 18, 2024 •

edited

Loading