From owner-freebsd-current@freebsd.org Fri Dec 2 16:20:37 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 5A33BC6270C for ; Fri, 2 Dec 2016 16:20:37 +0000 (UTC) (envelope-from vangyzen@FreeBSD.org) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 48E9F1DCE for ; Fri, 2 Dec 2016 16:20:37 +0000 (UTC) (envelope-from vangyzen@FreeBSD.org) Received: by mailman.ysv.freebsd.org (Postfix) id 483D1C6270B; Fri, 2 Dec 2016 16:20:37 +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 47E16C62709 for ; Fri, 2 Dec 2016 16:20:37 +0000 (UTC) (envelope-from vangyzen@FreeBSD.org) Received: from smtp.vangyzen.net (hotblack.vangyzen.net [IPv6:2607:fc50:1000:7400:216:3eff:fe72:314f]) by mx1.freebsd.org (Postfix) with ESMTP id 338D61DCC for ; Fri, 2 Dec 2016 16:20:37 +0000 (UTC) (envelope-from vangyzen@FreeBSD.org) Received: from ford.home.vangyzen.net (unknown [76.164.15.242]) by smtp.vangyzen.net (Postfix) with ESMTPSA id 78EB55648E; Fri, 2 Dec 2016 10:20:36 -0600 (CST) Subject: Re: copyinstr and ENAMETOOLONG To: Jilles Tjoelker References: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org> <20161102203354.GA22693@stack.nl> Cc: "current@freebsd.org" From: Eric van Gyzen Message-ID: <196a4cbf-b213-97bd-aae4-e761ff41e36b@FreeBSD.org> Date: Fri, 2 Dec 2016 10:20:32 -0600 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161102203354.GA22693@stack.nl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 16:20:37 -0000 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. > >> Index: kern_thr.c >> =================================================================== >> --- kern_thr.c (revision 308217) >> +++ kern_thr.c (working copy) >> @@ -580,8 +580,13 @@ 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. 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); }