Fixed get/set thread name implementations for macOS and FreeBSD

[YohDeadfall]

So, the story of fixing pthread_getname_np and pthread_setname_np continues, but this time I fixed the macOS implementation.

pthread_getname_np

The function never fails except for an invalid thread. Miri never verifies thread identifiers and uses them as indices when accessing a vector of threads. Therefore, the only possible result is 0 and a possibly trimmed output.

int
pthread_getname_np(pthread_t thread, char *threadname, size_t len)
{
	if (thread == pthread_self()) {
		strlcpy(threadname, thread->pthread_name, len);
		return 0;
	}


	if (!_pthread_validate_thread_and_list_lock(thread)) {
		return ESRCH;
	}


	strlcpy(threadname, thread->pthread_name, len);
	_pthread_lock_unlock(&_pthread_list_lock);
	return 0;
}

strcpy

strlcpy(3bsd)
strlcat(3bsd)
      Copy and catenate the input string into a destination
      string.  If the destination buffer, limited by its size,
      isn't large enough to hold the copy, the resulting string
      is truncated (but it is guaranteed to be null-terminated).
      They return the length of the total string they tried to
      create.

pthread_setname_np

pthread_setname_np(const char *name)
{
	int res;
	pthread_t self = pthread_self();


	size_t len = 0;
	if (name != NULL) {
		len = strlen(name);
	}


	_pthread_validate_signature(self);


	res = __proc_info(5, getpid(), 2, (uint64_t)0, (void*)name, (int)len);
	if (res == 0) {
		if (len > 0) {
			strlcpy(self->pthread_name, name, MAXTHREADNAMESIZE);
		} else {
			bzero(self->pthread_name, MAXTHREADNAMESIZE);
		}
	}
	return res;


}

Where 5 is PROC_INFO_CALL_SETCONTROL, and 2 is PROC_INFO_CALL_SETCONTROL. And __proc_info is a syscall handled by the XNU kernel by proc_info_internal:

int
proc_info_internal(int callnum, int pid, uint32_t flags, uint64_t ext_id, int flavor, uint64_t arg, user_addr_t buffer, uint32_t  buffersize, int32_t * retval)
{
	switch (callnum) {
	// ...
	case PROC_INFO_CALL_SETCONTROL:
		return proc_setcontrol(pid, flavor, arg, buffer, buffersize, retval);

And the actual logic from proc_setcontrol:

	case PROC_SELFSET_THREADNAME: {
		/*
		 * This is a bit ugly, as it copies the name into the kernel, and then
		 * invokes bsd_setthreadname again to copy it into the uthread name
		 * buffer.  Hopefully this isn't such a hot codepath that an additional
		 * MAXTHREADNAMESIZE copy is a big issue.
		 */
		if (buffersize > (MAXTHREADNAMESIZE - 1)) {
			return ENAMETOOLONG;
		}

Unrelated to the current pull request, but perhaps, there's a very ugly thing in the kernel/libc because the last thing happening in PROC_SELFSET_THREADNAME is bsd_setthreadname which sets the name in the user space. But we just saw that pthread_setname_np sets the name in the user space too. Guess, I need to open a ticket in one of Apple's repositories at least to clarify that :D

[RalfJung]

Thanks! Given how non-consistent the behavior is, I am not sure it is worth having the ERANGE in the shared code. Let's just return a bool, and every call site handles it in the way they see fit.

[RalfJung]

Can you also add a link to some source documenting that a truncated thread name does not lead to an error being returned? That seems like odd behavior...

[YohDeadfall]

Can you also add a link to some source documenting that a truncated thread name does not lead to an error being returned? That seems like odd behavior...

In my first message you can see that pthread_getname_np uses strlcpy, and there's documentation for it too. You can see that the only possible error is ESRCH, but that requires thread id validation. I can do that, but the current behavior is also fine because:

• Miri makes an out of bounds access to the threads vector and panics,
• it requires changes for other implementations then too, but it won't be correct anyway (you use a vector and indices, but for FreeBSH and macOS a thread id is a pointer to data).

Therefore, the only possible outcome for pthread_getname_np is 0.

[YohDeadfall]

Given how non-consistent the behavior is, I am not sure it is worth having the ERANGE in the shared code. Let's just return a bool, and every call site handles it in the way they see fit.

Mhm, let's keep erange suffixed functions for a while. I guess they can be used for my Android pull request as well, and maybe for other fixes too. If it wouldn't be so widely used, I will remove them.

[RalfJung]

In my first message you can see that pthread_getname_np uses strlcpy, and there's documentation for it too.

I asked for this to be included in the source, so that it's not lost to history. :)
Also, including the entire code isn't that useful. "Read the code" isn't good documentation.

Mhm, let's keep erange suffixed functions for a while.

No, let's not. I think it's nicer to require every caller to handle this explicitly.

[RalfJung]

Miri makes an out of bounds access to the threads vector and panics,

Oh, you can make Miri ICE with this? That's clearly a bug, please file an issue.

[YohDeadfall]

I asked for this to be included in the source, so that it's not lost to history. :)
Also, including the entire code isn't that useful. "Read the code" isn't good documentation.

Should it be a ling then to the source?

[RalfJung]

A link to the source, and a summary of what one sees there.

[RalfJung]

@rustbot author

[YohDeadfall]

@rustbot review

[RalfJung]

@rustbot author

[YohDeadfall]

@rustbot review

[RalfJung]

Thanks for gathering all the intel for all the targets. :) I guess we should have been more careful when adding these, the "np" should have been a warning that the behavior is really quite non-portable...

@rustbot author

[RalfJung]

Basically what I think we should have is a test for #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] where we call get_thread_name(&mut buf[..4]) and assert that we get back ERANGE.

[YohDeadfall]

I wouldn't add a test right now, because I haven't verified other platforms still. So, it would be misleading even if it looks correct according to the current interpreter logic. I also have some questions regarding FreeBSD, because it supports GNU extension method to get and set thread names, but they are not used by std at the moment while being present in libc, and they might have slightly different return codes.

[YohDeadfall]

Okay, had a quick look at Illumos which should be compatible with Solaris, and there's ERANGE too. Will do a test and write a comment.

[YohDeadfall]

Added just for Solaris like OSes because Linux has it's own already.

[YohDeadfall]

@rustbot review

@RalfJung, is it fine now for squashing and merging afterwards?

[RalfJung]

Yes please squash the commits. :) (use git rebase --keep-base for that to avoid a diff)

[RalfJung]

@rustbot author

[YohDeadfall]

Yes please squash the commits. :)

Squashed into two logical commits, so there's one for macOS and another for FreeBSD.

@rustbot review

[RalfJung]

Thanks! I have tweaked the test a little to have a more consistent shape.

@bors r+

[bors]

📌 Commit 027860e has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 027860e with merge 49b53d4...

[bors]

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