From owner-freebsd-current@freebsd.org Wed Nov 2 20:33:58 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 789F1C2B37A for ; Wed, 2 Nov 2016 20:33:58 +0000 (UTC) (envelope-from jilles@stack.nl) 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 0570C139F for ; Wed, 2 Nov 2016 20:33:58 +0000 (UTC) (envelope-from jilles@stack.nl) Received: by mailman.ysv.freebsd.org (Postfix) id 019EAC2B379; Wed, 2 Nov 2016 20:33:58 +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 012B5C2B378 for ; Wed, 2 Nov 2016 20:33:58 +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 94385139A; Wed, 2 Nov 2016 20:33:57 +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 236EA3B; Wed, 2 Nov 2016 21:33:55 +0100 (CET) Received: by snail.stack.nl (Postfix, from userid 1677) id 1541328494; Wed, 2 Nov 2016 21:33:55 +0100 (CET) Date: Wed, 2 Nov 2016 21:33:55 +0100 From: Jilles Tjoelker To: Eric van Gyzen Cc: "current@freebsd.org" Subject: Re: copyinstr and ENAMETOOLONG Message-ID: <20161102203354.GA22693@stack.nl> References: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@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: Wed, 02 Nov 2016 20:33:58 -0000 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) > @@ -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. A change to the man page thr_set_name(2) is needed in any case. -- Jilles Tjoelker