Re: copyinstr and ENAMETOOLONG

On Wed, Nov 02, 2016 at 02:24:43PM -0500, Eric van Gyzen wrote:
> Does copyinstr guarantee that it has filled the output buffer when it
> returns ENAMETOOLONG?  I usually try to answer my own questions, but I
> don't speak many dialects of assembly.  :)

The few I checked appear to work by checking the length while copying,
but the man page copy(9) does not guarantee that, and similar code in
sys/compat/linux/linux_misc.c linux_prctl() LINUX_PR_SET_NAME performs a
copyin() if copyinstr() fails with [ENAMETOOLONG].

In implementing copyinstr(), checking the length first may make sense
for economy of code: a user-strnlen() using an algorithm like
lib/libc/string/strlen.c and a copyin() connected together with a C
copyinstr(). This would probably be faster than the current amd64 code
(which uses lods and stos, meh). With that implementation, filling the
buffer in the [ENAMETOOLONG] case requires a small piece of additional
code.

> I ask because I'd like to make the following change, and I'd like to
> know whether I should zero the buffer before calling copyinstr to ensure
> that I don't set the thread's name to the garbage that was on the stack.

> Index: kern_thr.c
> ===================================================================
> --- kern_thr.c	(revision 308217)
> +++ kern_thr.c	(working copy)
> _at__at_ -580,8 +580,13 _at__at_ sys_thr_set_name(struct thread *td, struct thr_set
>  	if (uap->name != NULL) {
>  		error = copyinstr(uap->name, name, sizeof(name),
>  			NULL);
> -		if (error)
> -			return (error);
> +		if (error) {
> +			if (error == ENAMETOOLONG) {
> +				name[sizeof(name) - 1] = '\0';
> +			} else {
> +				return (error);
> +			}
> +		}
>  	}
>  	p = td->td_proc;
>  	ttd = tdfind((lwpid_t)uap->id, p->p_pid);

For API design, it makes more sense to set error = 0 if a truncated name
is being set anyway. This preserves the property that the name remains
unchanged if the call fails.

A change to the man page thr_set_name(2) is needed in any case.

-- 
Jilles Tjoelker