freebsd interceptions update proposal #3143

devnexen · 2023-10-26T19:38:51Z

No description provided.

RalfJung · 2023-10-26T21:08:16Z

Thanks for the PR!

However, new functions without tests are generally not a good idea. Why is the pthread_setname_np alias needed? And for pthread_get_name_np, we have a test in tests/pass-dep/shims/pthreads.rs. You could split out the thread name part of the test and enable it on freebsd (by editing ci.sh).

RalfJung · 2023-10-27T19:39:27Z

src/shims/unix/freebsd/foreign_items.rs

                let [thread, name] =
                    this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                let max_len = usize::MAX; // freebsd does not seem to have a limit.
+                let max_len = 20; // freebsd silently truncates to MAXCOMLEN (+ 1).


And MAXCOMLEN is 19 then, I assume?

Is there somewhere you can link to that documents this?

Sure
https://github.com/freebsd/freebsd-src/blob/314542de6d0a044640d349d68b2d1a1fe5f427ff/sys/sys/param.h#L127

here the syscall doc

https://github.com/freebsd/freebsd-src/blob/314542de6d0a044640d349d68b2d1a1fe5f427ff/lib/libc/sys/thr_set_name.2#L47

Okay, please add those links to the code. :)

RalfJung · 2023-10-27T19:41:14Z

tests/pass-dep/shims/pthreads.rs

+            libc::pthread_set_name_np(libc::pthread_self(), name.as_ptr().cast())
+        };
+        #[cfg(target_os = "freebsd")]
+        return 0;


Why do you make it return 0? With a too long name it should fail, no?

pthread_set_name_np functions differently than Linux's. The only errno returned to userland is ESRCH.

That's not what you implemented though. In src/shims/unix/freebsd/foreign_items.rs you're returning errors the same way Linux would.

Thanks for the suggestions.

RalfJung · 2023-10-27T19:43:14Z

tests/pass-dep/shims/pthreads.rs

+        #[cfg(target_os = "freebsd")]
+        unsafe {
+            libc::pthread_get_name_np(libc::pthread_self(), buf.as_mut_ptr().cast(), buf.len())
+        };
        let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
        assert!(cstr.to_bytes().len() >= 15); // POSIX seems to promise at least 15 chars


This assertion fails. Does it also fail locally when you do ./miri run --target x86_64-unknown-freebsd tests/pass-dep/shims/pthreads.rs? If yes then you have a shot at debugging this. Having the assert! print the actual length on failure would probably be a good start.

Suggested change

assert!(cstr.to_bytes().len() >= 15); // POSIX seems to promise at least 15 chars

assert!(cstr.to_bytes().len() >= 15, "name is too short: len={}", cstr.to_bytes().len()); // POSIX seems to promise at least 15 chars

RalfJung · 2023-10-27T19:57:06Z

src/shims/unix/freebsd/foreign_items.rs

                let [thread, name] =
                    this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                let max_len = usize::MAX; // freebsd does not seem to have a limit.
+                let max_len = 20; // freebsd silently truncates to MAXCOMLEN (+ 1).
                let res = this.pthread_setname_np(
                    this.read_scalar(thread)?,
                    this.read_scalar(name)?,
                    max_len,
                )?;
                this.write_scalar(res, dest)?;


I'm very surprised that this does not explode. Something is weird. This function returns nothing, and here we are assuming it returns an i32, so that should fail somewhere...

RalfJung · 2023-10-27T19:57:31Z

src/shims/unix/freebsd/foreign_items.rs

@@ -26,17 +26,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                    this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                this.write_null(dest)?;
            }
-            "pthread_set_name_np" => {
+            // Old vs new name
+            "pthread_set_name_np" | "pthread_setname_np" => {


According to the manpage these are not the same, they have different return types. So the implementation will also have to be different, and both variants need to be tested.

In fact I ll remove the new names for now and focus on the current ones, not even libc crate have them.

RalfJung

Sorry for the silence, I didn't realize this was ready for another round of review. Please don't just push and assume we'll get notified; github does not always send notifications for force pushes and it's never clear whether such a push is meant to resolve all prior discussions or whether more pushes will come.

@rustbot author
(write @rustbot review when this is ready for review again)

RalfJung · 2023-11-12T10:20:30Z

src/shims/unix/freebsd/foreign_items.rs

@@ -26,16 +26,26 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                    this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                this.write_null(dest)?;
            }
+            // Old vs new name


This comment seems outdated?

src/shims/unix/freebsd/foreign_items.rs

RalfJung · 2023-11-12T10:21:33Z

tests/pass-dep/shims/pthreads.rs

    test_mutex_libc_init_errorcheck();
+    #[cfg(not(target_os = "freebsd"))]
    test_rwlock_libc_static_initializer();
    test_named_thread_truncation();


Suggested change

test_named_thread_truncation();

test_named_thread_truncation();

RalfJung · 2023-11-12T10:22:20Z

tests/pass-dep/shims/pthreads.rs

+        unsafe {
+            libc::pthread_set_name_np(libc::pthread_self(), name.as_ptr().cast())
+        };
+        #[cfg(target_os = "freebsd")]
+        return 0;


Suggested change

unsafe {

libc::pthread_set_name_np(libc::pthread_self(), name.as_ptr().cast())

};

#[cfg(target_os = "freebsd")]

return 0;

unsafe {

libc::pthread_set_name_np(libc::pthread_self(), name.as_ptr().cast());

// pthread_set_name_np does not return anything, just assume it went well.

return 0;

};

RalfJung · 2023-11-12T10:23:15Z

tests/pass-dep/shims/pthreads.rs

        let cstr = CStr::from_bytes_until_nul(&buf).unwrap();
-        assert!(cstr.to_bytes().len() >= 15); // POSIX seems to promise at least 15 chars
+        assert!(cstr.to_bytes().len() >= 15, "name is too short: len={}", cstr.to_bytes().len()); // POSIX seems to promise at least 15 chars
        assert!(long_name.as_bytes().starts_with(cstr.to_bytes()));

        // Also test directly calling pthread_setname to check its return value.
        assert_eq!(set_thread_name(&cstr), 0);
        // But with a too long name it should fail.


Suggested change

// But with a too long name it should fail.

// But with a too long name it should fail (except on FreeBSD where the function

// has no return value and hence cannot indicate failure).

devnexen · 2023-11-12T11:53:39Z

@rustbot review

RalfJung · 2023-11-12T13:20:44Z

Looking good, thanks! I just did some tweaks.

@bors r+

bors · 2023-11-12T13:20:46Z

📌 Commit 19933b9 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2023-11-12T13:21:52Z

⌛ Testing commit 19933b9 with merge 485779a...

freebsd interceptions update proposal

RalfJung · 2023-11-12T13:23:14Z

@bors r+

bors · 2023-11-12T13:23:17Z

📌 Commit 195776b has been approved by RalfJung

It is now in the queue for this repository.

RalfJung · 2023-11-12T13:24:20Z

@bors force

bors · 2023-11-12T13:24:24Z

⌛ Testing commit 195776b with merge c30f30c...

bors · 2023-11-12T14:10:11Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing c30f30c to master...

devnexen force-pushed the fbsd_update branch 9 times, most recently from 3995215 to 7b9fc06 Compare October 27, 2023 06:49

RalfJung reviewed Oct 27, 2023

View reviewed changes

devnexen force-pushed the fbsd_update branch 3 times, most recently from 809057d to ab5ae00 Compare October 27, 2023 23:03

RalfJung reviewed Nov 12, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 12, 2023

devnexen force-pushed the fbsd_update branch 2 times, most recently from 2c6a1df to beacc3c Compare November 12, 2023 11:13

freebsd interceptions update proposal

bc154db

devnexen force-pushed the fbsd_update branch from beacc3c to bc154db Compare November 12, 2023 11:17

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 12, 2023

tweak comments

e0fef82

bors added a commit that referenced this pull request Nov 12, 2023

Auto merge of #3143 - devnexen:fbsd_update, r=RalfJung

485779a

freebsd interceptions update proposal

better error when calling pthread shims on unsupported unixes

195776b

RalfJung force-pushed the fbsd_update branch from 19933b9 to 195776b Compare November 12, 2023 13:23

bors merged commit c30f30c into rust-lang:master Nov 12, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

freebsd interceptions update proposal #3143

freebsd interceptions update proposal #3143

devnexen commented Oct 26, 2023

RalfJung commented Oct 26, 2023

RalfJung Oct 27, 2023

devnexen Oct 27, 2023

RalfJung Oct 27, 2023

devnexen Oct 27, 2023

RalfJung Oct 27, 2023

RalfJung Oct 27, 2023

devnexen Oct 27, 2023

RalfJung Oct 27, 2023

devnexen Oct 27, 2023

RalfJung Oct 27, 2023

RalfJung Oct 27, 2023

RalfJung Oct 27, 2023

devnexen Oct 27, 2023

RalfJung left a comment

RalfJung Nov 12, 2023

RalfJung Nov 12, 2023

RalfJung Nov 12, 2023

RalfJung Nov 12, 2023

devnexen commented Nov 12, 2023

RalfJung commented Nov 12, 2023

bors commented Nov 12, 2023

bors commented Nov 12, 2023

RalfJung commented Nov 12, 2023

bors commented Nov 12, 2023

RalfJung commented Nov 12, 2023

bors commented Nov 12, 2023

bors commented Nov 12, 2023

	assert!(cstr.to_bytes().len() >= 15); // POSIX seems to promise at least 15 chars
	assert!(cstr.to_bytes().len() >= 15, "name is too short: len={}", cstr.to_bytes().len()); // POSIX seems to promise at least 15 chars

	test_named_thread_truncation();

	test_named_thread_truncation();

	// But with a too long name it should fail.
	// But with a too long name it should fail (except on FreeBSD where the function
	// has no return value and hence cannot indicate failure).

freebsd interceptions update proposal #3143

freebsd interceptions update proposal #3143

Conversation

devnexen commented Oct 26, 2023

RalfJung commented Oct 26, 2023

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

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

RalfJung 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

devnexen commented Nov 12, 2023

RalfJung commented Nov 12, 2023

bors commented Nov 12, 2023

bors commented Nov 12, 2023

RalfJung commented Nov 12, 2023

bors commented Nov 12, 2023

RalfJung commented Nov 12, 2023

bors commented Nov 12, 2023

bors commented Nov 12, 2023