Date: Wed, 2 Nov 2016 21:33:55 +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: <20161102203354.GA22693@stack.nl> In-Reply-To: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org> References: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161102203354.GA22693>