Date: Sat, 3 Dec 2016 00:35:13 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Eric van Gyzen <vangyzen@FreeBSD.org> Cc: "current@freebsd.org" <current@freebsd.org> Subject: Re: copyinstr and ENAMETOOLONG Message-ID: <20161202233513.GA41141@stack.nl> In-Reply-To: <196a4cbf-b213-97bd-aae4-e761ff41e36b@FreeBSD.org> References: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org> <20161102203354.GA22693@stack.nl> <196a4cbf-b213-97bd-aae4-e761ff41e36b@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Dec 02, 2016 at 10:20:32AM -0600, Eric van Gyzen wrote:
> On 11/02/2016 15:33, Jilles Tjoelker wrote:
> > 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].
> Thanks.
> > 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.
> [snip]
> > 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.
> That makes sense.
> > A change to the man page thr_set_name(2) is needed in any case.
> Of course.
> Would you like to review the following?
> Index: lib/libc/sys/thr_set_name.2
> ===================================================================
> --- lib/libc/sys/thr_set_name.2 (revision 309300)
> +++ lib/libc/sys/thr_set_name.2 (working copy)
> @@ -28,7 +28,7 @@
> .\"
> .\" $FreeBSD$
> .\"
> -.Dd June 1, 2016
> +.Dd December 2, 2016
> .Dt THR_SET_NAME 2
> .Os
> .Sh NAME
> @@ -43,37 +43,34 @@
> .Sh DESCRIPTION
> The
> .Fn thr_set_name
> -sets the user-visible name for the kernel thread with the identifier
> +system call sets the user-visible name for the thread with the identifier
> .Va id
> -in the current process, to the NUL-terminated string
> +in the current process to the NUL-terminated string
> .Va name .
> +The name will be silently truncated to fit into a buffer of
> +.Dv MAXCOMLEN + 1
> +bytes.
> The thread name can be seen in the output of the
> .Xr ps 1
> and
> .Xr top 1
> commands, in the kernel debuggers and kernel tracing facility outputs,
> -also in userland debuggers and program core files, as notes.
> +and in userland debuggers and program core files, as notes.
> .Sh RETURN VALUES
> If successful,
> .Fn thr_set_name
> -will return zero, otherwise \-1 is returned, and
> +returns zero; otherwise, \-1 is returned, and
> .Va errno
> is set to indicate the error.
> .Sh ERRORS
> The
> .Fn thr_set_name
> -operation may return the following errors:
> +system call may return the following errors:
> .Bl -tag -width Er
> .It Bq Er EFAULT
> The memory pointed to by the
> .Fa name
> argument is not valid.
> -.It Bq Er ENAMETOOLONG
> -The string pointed to by the
> -.Fa name
> -argument exceeds
> -.Dv MAXCOMLEN + 1
> -bytes in length.
> .It Bq Er ESRCH
> The thread with the identifier
> .Fa id
> @@ -92,6 +89,6 @@ does not exist in the current process.
> .Xr ktr 9
> .Sh STANDARDS
> The
> -.Fn thr_new
> -system call is non-standard and is used by
> +.Fn thr_set_name
> +system call is non-standard and is used by the
> .Lb libthr .
> Index: share/man/man3/pthread_set_name_np.3
> ===================================================================
> --- share/man/man3/pthread_set_name_np.3 (revision 309300)
> +++ share/man/man3/pthread_set_name_np.3 (working copy)
> @@ -24,7 +24,7 @@
> .\"
> .\" $FreeBSD$
> .\"
> -.Dd February 13, 2003
> +.Dd December 2, 2016
> .Dt PTHREAD_SET_NAME_NP 3
> .Os
> .Sh NAME
> @@ -39,14 +39,15 @@
> .Sh DESCRIPTION
> The
> .Fn pthread_set_name_np
> -function sets internal name for thread specified by
> -.Fa tid
> -argument to string value specified by
> +function applies a copy of the given
> .Fa name
> -argument.
> +to the thread specified by the given
> +.Fa tid .
> .Sh ERRORS
> Because of the debugging nature of this function, all errors that may
> appear inside are silently ignored.
> +.Sh SEE ALSO
> +.Xr thr_set_name 2
> .Sh AUTHORS
> This manual page was written by
> .An Alexey Zelkin Aq Mt phantom@FreeBSD.org .
> Index: sys/kern/kern_thr.c
> ===================================================================
> --- sys/kern/kern_thr.c (revision 309300)
> +++ sys/kern/kern_thr.c (working copy)
> @@ -578,8 +578,11 @@ sys_thr_set_name(struct thread *td, struct thr_set
> error = 0;
> name[0] = '\0';
> if (uap->name != NULL) {
> - error = copyinstr(uap->name, name, sizeof(name),
> - NULL);
> + error = copyinstr(uap->name, name, sizeof(name), NULL);
> + if (error == ENAMETOOLONG) {
> + error = copyin(uap->name, name, sizeof(name) - 1);
> + name[sizeof(name) - 1] = '\0';
> + }
> if (error)
> return (error);
> }
Looks good to me. (I have not tested it.)
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161202233513.GA41141>