From owner-freebsd-current@freebsd.org Fri Dec 2 23:35:16 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id ADB37C638D0 for ; Fri, 2 Dec 2016 23:35:16 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 9AD0C966 for ; Fri, 2 Dec 2016 23:35:16 +0000 (UTC) (envelope-from jilles@stack.nl) Received: by mailman.ysv.freebsd.org (Postfix) id 976C8C638CC; Fri, 2 Dec 2016 23:35:16 +0000 (UTC) Delivered-To: current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 95551C638CA for ; Fri, 2 Dec 2016 23:35:16 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mailout.stack.nl (mailout05.stack.nl [IPv6:2001:610:1108:5010::202]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mailout.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 3E65B965; Fri, 2 Dec 2016 23:35:16 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mailout.stack.nl (Postfix) with ESMTP id 9AF8341EF; Sat, 3 Dec 2016 00:35:13 +0100 (CET) Received: by snail.stack.nl (Postfix, from userid 1677) id 8A16728494; Sat, 3 Dec 2016 00:35:13 +0100 (CET) Date: Sat, 3 Dec 2016 00:35:13 +0100 From: Jilles Tjoelker To: Eric van Gyzen Cc: "current@freebsd.org" Subject: Re: copyinstr and ENAMETOOLONG Message-ID: <20161202233513.GA41141@stack.nl> References: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org> <20161102203354.GA22693@stack.nl> <196a4cbf-b213-97bd-aae4-e761ff41e36b@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <196a4cbf-b213-97bd-aae4-e761ff41e36b@FreeBSD.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Dec 2016 23:35:16 -0000 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