Re: copyinstr and ENAMETOOLONG
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 SE
Re: copyinstr and ENAMETOOLONG
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 valu
Re: copyinstr and ENAMETOOLONG
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
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
copyinstr and ENAMETOOLONG
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. :)
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.
Eric
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);
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
