Date: Fri, 2 Dec 2016 10:20:32 -0600 From: Eric van Gyzen <vangyzen@FreeBSD.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: "current@freebsd.org" <current@freebsd.org> Subject: Re: copyinstr and ENAMETOOLONG Message-ID: <196a4cbf-b213-97bd-aae4-e761ff41e36b@FreeBSD.org> In-Reply-To: <20161102203354.GA22693@stack.nl> References: <236b8c7c-a12e-0872-f3cb-03f99bb5fcc5@FreeBSD.org> <20161102203354.GA22693@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?196a4cbf-b213-97bd-aae4-e761ff41e36b>