Message ID | 20230707200400.378096-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | getpw: Get rid of alloca | expand |
On Fri, Jul 07, 2023 at 04:04:00PM -0400, Joe Simmons-Talbott wrote: > Use a scratch_buffer rather than alloca to avoid potential stack > overflow. Ping. Thanks, Joe > --- > pwd/getpw.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/pwd/getpw.c b/pwd/getpw.c > index cf747374b8..7a27d79910 100644 > --- a/pwd/getpw.c > +++ b/pwd/getpw.c > @@ -15,8 +15,8 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include <alloca.h> > #include <errno.h> > +#include <scratch_buffer.h> > #include <stdio.h> > #include <unistd.h> > #include <pwd.h> > @@ -34,28 +34,48 @@ __getpw (__uid_t uid, char *buf) > size_t buflen; > char *tmpbuf; > struct passwd resbuf, *p; > + int retval = 0; > + struct scratch_buffer sbuf; > + scratch_buffer_init (&sbuf); > > if (buf == NULL) > { > __set_errno (EINVAL); > - return -1; > + retval = -1; > + goto error_out; > } > > buflen = __sysconf (_SC_GETPW_R_SIZE_MAX); > - tmpbuf = alloca (buflen); > + if (!scratch_buffer_set_array_size (&sbuf, 1, buflen)) > + { > + retval = -1; > + goto error_out; > + } > + tmpbuf = sbuf.data; > > if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0) > - return -1; > + { > + retval = -1; > + goto error_out; > + } > > if (p == NULL) > - return -1; > + { > + retval = -1; > + goto error_out; > + } > > if (sprintf (buf, "%s:%s:%lu:%lu:%s:%s:%s", p->pw_name, p->pw_passwd, > (unsigned long int) p->pw_uid, (unsigned long int) p->pw_gid, > p->pw_gecos, p->pw_dir, p->pw_shell) < 0) > - return -1; > + { > + retval = -1; > + goto error_out; > + } > > - return 0; > +error_out: > + scratch_buffer_free (&sbuf); > + return retval; > } > weak_alias (__getpw, getpw) > > -- > 2.39.2 >
Ping. On Thu, Aug 10, 2023 at 09:45:47AM -0400, Joe Simmons-Talbott via Libc-alpha wrote: > On Fri, Jul 07, 2023 at 04:04:00PM -0400, Joe Simmons-Talbott wrote: > > Use a scratch_buffer rather than alloca to avoid potential stack > > overflow. > > Ping. > > Thanks, > Joe > > --- > > pwd/getpw.c | 34 +++++++++++++++++++++++++++------- > > 1 file changed, 27 insertions(+), 7 deletions(-) > > > > diff --git a/pwd/getpw.c b/pwd/getpw.c > > index cf747374b8..7a27d79910 100644 > > --- a/pwd/getpw.c > > +++ b/pwd/getpw.c > > @@ -15,8 +15,8 @@ > > License along with the GNU C Library; if not, see > > <https://www.gnu.org/licenses/>. */ > > > > -#include <alloca.h> > > #include <errno.h> > > +#include <scratch_buffer.h> > > #include <stdio.h> > > #include <unistd.h> > > #include <pwd.h> > > @@ -34,28 +34,48 @@ __getpw (__uid_t uid, char *buf) > > size_t buflen; > > char *tmpbuf; > > struct passwd resbuf, *p; > > + int retval = 0; > > + struct scratch_buffer sbuf; > > + scratch_buffer_init (&sbuf); > > > > if (buf == NULL) > > { > > __set_errno (EINVAL); > > - return -1; > > + retval = -1; > > + goto error_out; > > } > > > > buflen = __sysconf (_SC_GETPW_R_SIZE_MAX); > > - tmpbuf = alloca (buflen); > > + if (!scratch_buffer_set_array_size (&sbuf, 1, buflen)) > > + { > > + retval = -1; > > + goto error_out; > > + } > > + tmpbuf = sbuf.data; > > > > if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0) > > - return -1; > > + { > > + retval = -1; > > + goto error_out; > > + } > > > > if (p == NULL) > > - return -1; > > + { > > + retval = -1; > > + goto error_out; > > + } > > > > if (sprintf (buf, "%s:%s:%lu:%lu:%s:%s:%s", p->pw_name, p->pw_passwd, > > (unsigned long int) p->pw_uid, (unsigned long int) p->pw_gid, > > p->pw_gecos, p->pw_dir, p->pw_shell) < 0) > > - return -1; > > + { > > + retval = -1; > > + goto error_out; > > + } > > > > - return 0; > > +error_out: > > + scratch_buffer_free (&sbuf); > > + return retval; > > } > > weak_alias (__getpw, getpw) > > > > -- > > 2.39.2 > > >
On 07/07/23 17:04, Joe Simmons-Talbott via Libc-alpha wrote: > Use a scratch_buffer rather than alloca to avoid potential stack > overflow. > --- > pwd/getpw.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/pwd/getpw.c b/pwd/getpw.c > index cf747374b8..7a27d79910 100644 > --- a/pwd/getpw.c > +++ b/pwd/getpw.c > @@ -15,8 +15,8 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include <alloca.h> > #include <errno.h> > +#include <scratch_buffer.h> > #include <stdio.h> > #include <unistd.h> > #include <pwd.h> > @@ -34,28 +34,48 @@ __getpw (__uid_t uid, char *buf) > size_t buflen; > char *tmpbuf; > struct passwd resbuf, *p; > + int retval = 0; > + struct scratch_buffer sbuf; > + scratch_buffer_init (&sbuf); > > if (buf == NULL) > { > __set_errno (EINVAL); > - return -1; > + retval = -1; > + goto error_out; > } > There is no need to call scratch_buffer_free here. You can move the scratch_buffer initialization later. > buflen = __sysconf (_SC_GETPW_R_SIZE_MAX); > - tmpbuf = alloca (buflen); > + if (!scratch_buffer_set_array_size (&sbuf, 1, buflen)) The _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD so there is no need to a scratch_buffer here (similar to sysdeps/posix/cuserid.c assumption). Since the functions is historical tricky to be used correctly, I think it should continue to fail with passwords larger than _SC_GETPW_R_SIZE_MAX. > + { > + retval = -1; > + goto error_out; > + } > + tmpbuf = sbuf.data; > > if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0) > - return -1; > + { > + retval = -1; > + goto error_out; > + } > > if (p == NULL) > - return -1; > + { > + retval = -1; > + goto error_out; > + } > > if (sprintf (buf, "%s:%s:%lu:%lu:%s:%s:%s", p->pw_name, p->pw_passwd, > (unsigned long int) p->pw_uid, (unsigned long int) p->pw_gid, > p->pw_gecos, p->pw_dir, p->pw_shell) < 0) > - return -1; > + { > + retval = -1; > + goto error_out; > + } > > - return 0; > +error_out: > + scratch_buffer_free (&sbuf); > + return retval; > } > weak_alias (__getpw, getpw) >
On Mon, Aug 28, 2023 at 02:01:54PM -0300, Adhemerval Zanella Netto wrote: > > > On 07/07/23 17:04, Joe Simmons-Talbott via Libc-alpha wrote: > > Use a scratch_buffer rather than alloca to avoid potential stack > > overflow. > > --- > > pwd/getpw.c | 34 +++++++++++++++++++++++++++------- > > 1 file changed, 27 insertions(+), 7 deletions(-) > > > > diff --git a/pwd/getpw.c b/pwd/getpw.c > > index cf747374b8..7a27d79910 100644 > > --- a/pwd/getpw.c > > +++ b/pwd/getpw.c > > @@ -15,8 +15,8 @@ > > License along with the GNU C Library; if not, see > > <https://www.gnu.org/licenses/>. */ > > > > -#include <alloca.h> > > #include <errno.h> > > +#include <scratch_buffer.h> > > #include <stdio.h> > > #include <unistd.h> > > #include <pwd.h> > > @@ -34,28 +34,48 @@ __getpw (__uid_t uid, char *buf) > > size_t buflen; > > char *tmpbuf; > > struct passwd resbuf, *p; > > + int retval = 0; > > + struct scratch_buffer sbuf; > > + scratch_buffer_init (&sbuf); > > > > if (buf == NULL) > > { > > __set_errno (EINVAL); > > - return -1; > > + retval = -1; > > + goto error_out; > > } > > > > There is no need to call scratch_buffer_free here. You can move the > scratch_buffer initialization later. > > > buflen = __sysconf (_SC_GETPW_R_SIZE_MAX); > > - tmpbuf = alloca (buflen); > > + if (!scratch_buffer_set_array_size (&sbuf, 1, buflen)) > > The _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD so there is no need to > a scratch_buffer here (similar to sysdeps/posix/cuserid.c assumption). Since the > functions is historical tricky to be used correctly, I think it should continue to > fail with passwords larger than _SC_GETPW_R_SIZE_MAX. I removed the scratch_buffer in v2 and used a fixed sized array. Thanks, Joe > > > + { > > + retval = -1; > > + goto error_out; > > + } > > + tmpbuf = sbuf.data; > > > > if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0) > > - return -1; > > + { > > + retval = -1; > > + goto error_out; > > + } > > > > if (p == NULL) > > - return -1; > > + { > > + retval = -1; > > + goto error_out; > > + } > > > > if (sprintf (buf, "%s:%s:%lu:%lu:%s:%s:%s", p->pw_name, p->pw_passwd, > > (unsigned long int) p->pw_uid, (unsigned long int) p->pw_gid, > > p->pw_gecos, p->pw_dir, p->pw_shell) < 0) > > - return -1; > > + { > > + retval = -1; > > + goto error_out; > > + } > > > > - return 0; > > +error_out: > > + scratch_buffer_free (&sbuf); > > + return retval; > > } > > weak_alias (__getpw, getpw) > > >
diff --git a/pwd/getpw.c b/pwd/getpw.c index cf747374b8..7a27d79910 100644 --- a/pwd/getpw.c +++ b/pwd/getpw.c @@ -15,8 +15,8 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <alloca.h> #include <errno.h> +#include <scratch_buffer.h> #include <stdio.h> #include <unistd.h> #include <pwd.h> @@ -34,28 +34,48 @@ __getpw (__uid_t uid, char *buf) size_t buflen; char *tmpbuf; struct passwd resbuf, *p; + int retval = 0; + struct scratch_buffer sbuf; + scratch_buffer_init (&sbuf); if (buf == NULL) { __set_errno (EINVAL); - return -1; + retval = -1; + goto error_out; } buflen = __sysconf (_SC_GETPW_R_SIZE_MAX); - tmpbuf = alloca (buflen); + if (!scratch_buffer_set_array_size (&sbuf, 1, buflen)) + { + retval = -1; + goto error_out; + } + tmpbuf = sbuf.data; if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0) - return -1; + { + retval = -1; + goto error_out; + } if (p == NULL) - return -1; + { + retval = -1; + goto error_out; + } if (sprintf (buf, "%s:%s:%lu:%lu:%s:%s:%s", p->pw_name, p->pw_passwd, (unsigned long int) p->pw_uid, (unsigned long int) p->pw_gid, p->pw_gecos, p->pw_dir, p->pw_shell) < 0) - return -1; + { + retval = -1; + goto error_out; + } - return 0; +error_out: + scratch_buffer_free (&sbuf); + return retval; } weak_alias (__getpw, getpw)