* [PATCH] grantpt: Get rid of alloca @ 2023-06-01 19:58 Joe Simmons-Talbott 2023-06-05 13:29 ` Florian Weimer 0 siblings, 1 reply; 8+ messages in thread From: Joe Simmons-Talbott @ 2023-06-01 19:58 UTC (permalink / raw) To: libc-alpha; +Cc: Joe Simmons-Talbott Replace alloca with a scratch_buffer to avoid potential stack overflows. --- sysdeps/unix/grantpt.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c index 38fce52576..e5d2390bf2 100644 --- a/sysdeps/unix/grantpt.c +++ b/sysdeps/unix/grantpt.c @@ -20,6 +20,7 @@ #include <fcntl.h> #include <grp.h> #include <limits.h> +#include <scratch_buffer.h> #include <stdlib.h> #include <string.h> #include <sys/resource.h> @@ -147,7 +148,14 @@ grantpt (int fd) /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. Try a moderate value. */ grbuflen = 1024; - grtmpbuf = (char *) __alloca (grbuflen); + struct scratch_buffer sbuf; + scratch_buffer_init (&sbuf); + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) + { + retval -1; + goto cleanup; + } + grtmpbuf = sbuf.data; __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); if (p != NULL) tty_gid = p->gr_gid; @@ -255,6 +263,8 @@ grantpt (int fd) if (buf != _buf) free (buf); + scratch_buffer_free(sbuf); + return retval; } libc_hidden_def (grantpt) -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-01 19:58 [PATCH] grantpt: Get rid of alloca Joe Simmons-Talbott @ 2023-06-05 13:29 ` Florian Weimer 2023-06-05 13:37 ` Joe Simmons-Talbott 2023-06-07 0:40 ` Samuel Thibault 0 siblings, 2 replies; 8+ messages in thread From: Florian Weimer @ 2023-06-05 13:29 UTC (permalink / raw) To: Joe Simmons-Talbott via Libc-alpha Cc: Joe Simmons-Talbott, Samuel Thibault, Sergey Bugaev * Joe Simmons-Talbott via Libc-alpha: > Replace alloca with a scratch_buffer to avoid potential stack overflows. > --- > sysdeps/unix/grantpt.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c > index 38fce52576..e5d2390bf2 100644 > --- a/sysdeps/unix/grantpt.c > +++ b/sysdeps/unix/grantpt.c > @@ -20,6 +20,7 @@ > #include <fcntl.h> > #include <grp.h> > #include <limits.h> > +#include <scratch_buffer.h> > #include <stdlib.h> > #include <string.h> > #include <sys/resource.h> > @@ -147,7 +148,14 @@ grantpt (int fd) > /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. > Try a moderate value. */ > grbuflen = 1024; > - grtmpbuf = (char *) __alloca (grbuflen); > + struct scratch_buffer sbuf; > + scratch_buffer_init (&sbuf); > + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) > + { > + retval -1; > + goto cleanup; > + } > + grtmpbuf = sbuf.data; > __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); > if (p != NULL) > tty_gid = p->gr_gid; > @@ -255,6 +263,8 @@ grantpt (int fd) > if (buf != _buf) > free (buf); > > + scratch_buffer_free(sbuf); > + > return retval; > } > libc_hidden_def (grantpt) How did you test this? This code is only used on Hurd due to the override in sysdeps/unix/sysv/linux/grantpt.c. Maybe the hurd maintainers could review this change? Thanks, Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-05 13:29 ` Florian Weimer @ 2023-06-05 13:37 ` Joe Simmons-Talbott 2023-06-05 18:14 ` Adhemerval Zanella Netto 2023-06-07 0:40 ` Samuel Thibault 1 sibling, 1 reply; 8+ messages in thread From: Joe Simmons-Talbott @ 2023-06-05 13:37 UTC (permalink / raw) To: Florian Weimer Cc: Joe Simmons-Talbott via Libc-alpha, Samuel Thibault, Sergey Bugaev On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote: > * Joe Simmons-Talbott via Libc-alpha: > > > Replace alloca with a scratch_buffer to avoid potential stack overflows. > > --- > > sysdeps/unix/grantpt.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c > > index 38fce52576..e5d2390bf2 100644 > > --- a/sysdeps/unix/grantpt.c > > +++ b/sysdeps/unix/grantpt.c > > @@ -20,6 +20,7 @@ > > #include <fcntl.h> > > #include <grp.h> > > #include <limits.h> > > +#include <scratch_buffer.h> > > #include <stdlib.h> > > #include <string.h> > > #include <sys/resource.h> > > @@ -147,7 +148,14 @@ grantpt (int fd) > > /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. > > Try a moderate value. */ > > grbuflen = 1024; > > - grtmpbuf = (char *) __alloca (grbuflen); > > + struct scratch_buffer sbuf; > > + scratch_buffer_init (&sbuf); > > + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) > > + { > > + retval -1; > > + goto cleanup; > > + } > > + grtmpbuf = sbuf.data; > > __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); > > if (p != NULL) > > tty_gid = p->gr_gid; > > @@ -255,6 +263,8 @@ grantpt (int fd) > > if (buf != _buf) > > free (buf); > > > > + scratch_buffer_free(sbuf); > > + > > return retval; > > } > > libc_hidden_def (grantpt) > > How did you test this? This code is only used on Hurd due to the > override in sysdeps/unix/sysv/linux/grantpt.c. The only testing I did was a 'make && make check' on x86_64 and i686 linux. Thanks, Joe > > Maybe the hurd maintainers could review this change? > > Thanks, > Florian > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-05 13:37 ` Joe Simmons-Talbott @ 2023-06-05 18:14 ` Adhemerval Zanella Netto 2023-06-05 22:02 ` Joe Simmons-Talbott 0 siblings, 1 reply; 8+ messages in thread From: Adhemerval Zanella Netto @ 2023-06-05 18:14 UTC (permalink / raw) To: Joe Simmons-Talbott, Florian Weimer, Siddhesh Poyarekar Cc: Joe Simmons-Talbott via Libc-alpha, Samuel Thibault, Sergey Bugaev On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote: > On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote: >> * Joe Simmons-Talbott via Libc-alpha: >> >>> Replace alloca with a scratch_buffer to avoid potential stack overflows. >>> --- >>> sysdeps/unix/grantpt.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c >>> index 38fce52576..e5d2390bf2 100644 >>> --- a/sysdeps/unix/grantpt.c >>> +++ b/sysdeps/unix/grantpt.c >>> @@ -20,6 +20,7 @@ >>> #include <fcntl.h> >>> #include <grp.h> >>> #include <limits.h> >>> +#include <scratch_buffer.h> >>> #include <stdlib.h> >>> #include <string.h> >>> #include <sys/resource.h> >>> @@ -147,7 +148,14 @@ grantpt (int fd) >>> /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. >>> Try a moderate value. */ >>> grbuflen = 1024; >>> - grtmpbuf = (char *) __alloca (grbuflen); >>> + struct scratch_buffer sbuf; >>> + scratch_buffer_init (&sbuf); >>> + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) >>> + { >>> + retval -1; >>> + goto cleanup; >>> + } >>> + grtmpbuf = sbuf.data; >>> __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); >>> if (p != NULL) >>> tty_gid = p->gr_gid; >>> @@ -255,6 +263,8 @@ grantpt (int fd) >>> if (buf != _buf) >>> free (buf); >>> >>> + scratch_buffer_free(sbuf); >>> + >>> return retval; >>> } >>> libc_hidden_def (grantpt) >> >> How did you test this? This code is only used on Hurd due to the >> override in sysdeps/unix/sysv/linux/grantpt.c. > > The only testing I did was a 'make && make check' on x86_64 and i686 linux. You will need to build for i686-gnu (Hurd) to actually enable this code. Which leads to another, which Siddheash has brough, which is --enable-pt_chown does make sense to be provided as a configure switch. I think we should just move all pt_chown to be Hurd specific, and remove the --enable-pt_chown option. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-05 18:14 ` Adhemerval Zanella Netto @ 2023-06-05 22:02 ` Joe Simmons-Talbott 2023-06-06 6:14 ` Siddhesh Poyarekar 0 siblings, 1 reply; 8+ messages in thread From: Joe Simmons-Talbott @ 2023-06-05 22:02 UTC (permalink / raw) To: Adhemerval Zanella Netto Cc: Florian Weimer, Siddhesh Poyarekar, Joe Simmons-Talbott via Libc-alpha, Samuel Thibault, Sergey Bugaev On Mon, Jun 05, 2023 at 03:14:06PM -0300, Adhemerval Zanella Netto wrote: > > > On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote: > > On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote: > >> * Joe Simmons-Talbott via Libc-alpha: > >> > >>> Replace alloca with a scratch_buffer to avoid potential stack overflows. > >>> --- > >>> sysdeps/unix/grantpt.c | 12 +++++++++++- > >>> 1 file changed, 11 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c > >>> index 38fce52576..e5d2390bf2 100644 > >>> --- a/sysdeps/unix/grantpt.c > >>> +++ b/sysdeps/unix/grantpt.c > >>> @@ -20,6 +20,7 @@ > >>> #include <fcntl.h> > >>> #include <grp.h> > >>> #include <limits.h> > >>> +#include <scratch_buffer.h> > >>> #include <stdlib.h> > >>> #include <string.h> > >>> #include <sys/resource.h> > >>> @@ -147,7 +148,14 @@ grantpt (int fd) > >>> /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. > >>> Try a moderate value. */ > >>> grbuflen = 1024; > >>> - grtmpbuf = (char *) __alloca (grbuflen); > >>> + struct scratch_buffer sbuf; > >>> + scratch_buffer_init (&sbuf); > >>> + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) > >>> + { > >>> + retval -1; > >>> + goto cleanup; > >>> + } > >>> + grtmpbuf = sbuf.data; > >>> __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); > >>> if (p != NULL) > >>> tty_gid = p->gr_gid; > >>> @@ -255,6 +263,8 @@ grantpt (int fd) > >>> if (buf != _buf) > >>> free (buf); > >>> > >>> + scratch_buffer_free(sbuf); > >>> + > >>> return retval; > >>> } > >>> libc_hidden_def (grantpt) > >> > >> How did you test this? This code is only used on Hurd due to the > >> override in sysdeps/unix/sysv/linux/grantpt.c. > > > > The only testing I did was a 'make && make check' on x86_64 and i686 linux. > > You will need to build for i686-gnu (Hurd) to actually enable this code. Which > leads to another, which Siddheash has brough, which is --enable-pt_chown does > make sense to be provided as a configure switch. I tested this patch today with build-many-glibcs.py for i686-gnu and it passed 'make check'. Does your second sentence mean that I needed to pass '--enable-pt_chown' to configure for the test build? Thanks, Joe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-05 22:02 ` Joe Simmons-Talbott @ 2023-06-06 6:14 ` Siddhesh Poyarekar 2023-06-07 19:05 ` Zack Weinberg 0 siblings, 1 reply; 8+ messages in thread From: Siddhesh Poyarekar @ 2023-06-06 6:14 UTC (permalink / raw) To: Joe Simmons-Talbott, Adhemerval Zanella Netto Cc: Florian Weimer, Joe Simmons-Talbott via Libc-alpha, Samuel Thibault, Sergey Bugaev On 2023-06-05 18:02, Joe Simmons-Talbott wrote: > On Mon, Jun 05, 2023 at 03:14:06PM -0300, Adhemerval Zanella Netto wrote: >> >> >> On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote: >>> On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote: >>>> * Joe Simmons-Talbott via Libc-alpha: >>>> >>>>> Replace alloca with a scratch_buffer to avoid potential stack overflows. >>>>> --- >>>>> sysdeps/unix/grantpt.c | 12 +++++++++++- >>>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c >>>>> index 38fce52576..e5d2390bf2 100644 >>>>> --- a/sysdeps/unix/grantpt.c >>>>> +++ b/sysdeps/unix/grantpt.c >>>>> @@ -20,6 +20,7 @@ >>>>> #include <fcntl.h> >>>>> #include <grp.h> >>>>> #include <limits.h> >>>>> +#include <scratch_buffer.h> >>>>> #include <stdlib.h> >>>>> #include <string.h> >>>>> #include <sys/resource.h> >>>>> @@ -147,7 +148,14 @@ grantpt (int fd) >>>>> /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. >>>>> Try a moderate value. */ >>>>> grbuflen = 1024; >>>>> - grtmpbuf = (char *) __alloca (grbuflen); >>>>> + struct scratch_buffer sbuf; >>>>> + scratch_buffer_init (&sbuf); >>>>> + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) >>>>> + { >>>>> + retval -1; >>>>> + goto cleanup; >>>>> + } >>>>> + grtmpbuf = sbuf.data; >>>>> __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); >>>>> if (p != NULL) >>>>> tty_gid = p->gr_gid; >>>>> @@ -255,6 +263,8 @@ grantpt (int fd) >>>>> if (buf != _buf) >>>>> free (buf); >>>>> >>>>> + scratch_buffer_free(sbuf); >>>>> + >>>>> return retval; >>>>> } >>>>> libc_hidden_def (grantpt) >>>> >>>> How did you test this? This code is only used on Hurd due to the >>>> override in sysdeps/unix/sysv/linux/grantpt.c. >>> >>> The only testing I did was a 'make && make check' on x86_64 and i686 linux. >> >> You will need to build for i686-gnu (Hurd) to actually enable this code. Which >> leads to another, which Siddheash has brough, which is --enable-pt_chown does >> make sense to be provided as a configure switch. Yeah I looked at the code again and perhaps it's something for Samuel to think about. We don't seem to have target-specific configure options, so it's a nop for Linux. > I tested this patch today with build-many-glibcs.py for i686-gnu and it > passed 'make check'. Does your second sentence mean that I needed to > pass '--enable-pt_chown' to configure for the test build? Not really, the code you're changing is outside of HAVE_PT_CHOWN, so you're good to go I think. I'd wait for Samuel to ack this since this is exclusively hurd code. Thanks, Sid ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-06 6:14 ` Siddhesh Poyarekar @ 2023-06-07 19:05 ` Zack Weinberg 0 siblings, 0 replies; 8+ messages in thread From: Zack Weinberg @ 2023-06-07 19:05 UTC (permalink / raw) To: GNU libc development On Tue, Jun 6, 2023, at 2:14 AM, Siddhesh Poyarekar wrote: ... >>>>> How did you test this? This code is only used on Hurd due to the >>>>> override in sysdeps/unix/sysv/linux/grantpt.c. >>>> >>>> The only testing I did was a 'make && make check' on x86_64 and >>>> i686 linux. >>> >>> You will need to build for i686-gnu (Hurd) to actually enable this >>> code. Which leads to another, which Siddheash has brough, which is >>> --enable-pt_chown does make sense to be provided as a configure >>> switch. > > Yeah I looked at the code again and perhaps it's something for Samuel > to think about. We don't seem to have target-specific configure > options, so it's a nop for Linux. pt_chown has been troublesome in the past (up to and including privilege escalation vulnerabilities) and, iirc, it's the only helper binary we build that needs to be setuid. It would be nice if we could get rid of it altogether. I believe the requirements for promoting the Linux- specific code to generic (or just all-GNU) are for the Hurd to implement a /dev/pts virtual file system that automatically adjusts the ownership and permissions of /dev/pts/NNN when someone opens /dev/ptmx, and additionally for it to add support for whichever of the TIOCSPTLCK, TIOCGPTLCK, and TIOCGPTPEER ioctls are not already implemented. zw ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] grantpt: Get rid of alloca 2023-06-05 13:29 ` Florian Weimer 2023-06-05 13:37 ` Joe Simmons-Talbott @ 2023-06-07 0:40 ` Samuel Thibault 1 sibling, 0 replies; 8+ messages in thread From: Samuel Thibault @ 2023-06-07 0:40 UTC (permalink / raw) To: Florian Weimer Cc: Joe Simmons-Talbott via Libc-alpha, Joe Simmons-Talbott, Sergey Bugaev Hello, * Joe Simmons-Talbott via Libc-alpha: > > Replace alloca with a scratch_buffer to avoid potential stack overflows. > > --- > > sysdeps/unix/grantpt.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c > > index 38fce52576..e5d2390bf2 100644 > > --- a/sysdeps/unix/grantpt.c > > +++ b/sysdeps/unix/grantpt.c > > @@ -147,7 +148,14 @@ grantpt (int fd) > > /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. > > Try a moderate value. */ > > grbuflen = 1024; > > - grtmpbuf = (char *) __alloca (grbuflen); > > + struct scratch_buffer sbuf; > > + scratch_buffer_init (&sbuf); > > + if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen)) > > + { > > + retval -1; This does not compile, missing = > > + goto cleanup; > > + } > > + grtmpbuf = sbuf.data; > > __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p); > > if (p != NULL) > > tty_gid = p->gr_gid; > > @@ -255,6 +263,8 @@ grantpt (int fd) > > if (buf != _buf) > > free (buf); > > > > + scratch_buffer_free(sbuf); sbuf is undefined here since it was defined in the if (__glibc_unlikely (tty_gid == -1)) block. Probably you just want to just move the scratch_buffer_free call to the end of that block. Also, you need to pass &sbuf, not sbuf. Samuel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-07 19:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-01 19:58 [PATCH] grantpt: Get rid of alloca Joe Simmons-Talbott 2023-06-05 13:29 ` Florian Weimer 2023-06-05 13:37 ` Joe Simmons-Talbott 2023-06-05 18:14 ` Adhemerval Zanella Netto 2023-06-05 22:02 ` Joe Simmons-Talbott 2023-06-06 6:14 ` Siddhesh Poyarekar 2023-06-07 19:05 ` Zack Weinberg 2023-06-07 0:40 ` Samuel Thibault
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).