Message ID | 20230828194911.877648-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] getpw: Get rid of alloca | expand |
* Joe Simmons-Talbott via Libc-alpha: > Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed > sized array rather than alloca to avoid potential stack overflow. _SC_GETPW_R_SIZE_MAX is not very well-named, it is the initial suggested buffer size. The code should use the usual scratch buffer retry loop. Thanks, Florian
On 29/08/23 05:03, Florian Weimer via Libc-alpha wrote: > * Joe Simmons-Talbott via Libc-alpha: > >> Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed >> sized array rather than alloca to avoid potential stack overflow. > > _SC_GETPW_R_SIZE_MAX is not very well-named, it is the initial suggested > buffer size. The code should use the usual scratch buffer retry loop. I though about that on initial revision, however this will change the function semantic and the code below: long int sz = sysconf (_SC_GETPW_R_SIZE_MAX); char *buf = NULL; while (1) { buf = realloc (buf, sz); r = getpw (uid, buf); if (r != -1 || errno != ENOMEM) break; sz *= 2; } will start to trigger a buffer overrun. This is an old tricky interface where I would prefer if we continue to keep same semantic, and maybe deprecate and move it to compat symbol.
On Tue, Aug 29, 2023 at 09:26:39AM -0300, Adhemerval Zanella Netto via Libc-alpha wrote: > > > On 29/08/23 05:03, Florian Weimer via Libc-alpha wrote: > > * Joe Simmons-Talbott via Libc-alpha: > > > >> Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed > >> sized array rather than alloca to avoid potential stack overflow. > > > > _SC_GETPW_R_SIZE_MAX is not very well-named, it is the initial suggested > > buffer size. The code should use the usual scratch buffer retry loop. > > I though about that on initial revision, however this will change the function > semantic and the code below: > > long int sz = sysconf (_SC_GETPW_R_SIZE_MAX); > char *buf = NULL; > while (1) { > buf = realloc (buf, sz); > r = getpw (uid, buf); > if (r != -1 || errno != ENOMEM) > break; > sz *= 2; > } > > will start to trigger a buffer overrun. This is an old tricky interface > where I would prefer if we continue to keep same semantic, and maybe > deprecate and move it to compat symbol. > Where does this leave this patch? Should I submit a patch deprecating getpw? Thanks, Joe
diff --git a/pwd/getpw.c b/pwd/getpw.c index cf747374b8..86ccbc8d6e 100644 --- a/pwd/getpw.c +++ b/pwd/getpw.c @@ -15,7 +15,6 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <alloca.h> #include <errno.h> #include <stdio.h> #include <unistd.h> @@ -31,31 +30,39 @@ int __getpw (__uid_t uid, char *buf); int __getpw (__uid_t uid, char *buf) { - size_t buflen; - char *tmpbuf; + char tmpbuf[NSS_BUFLEN_PASSWD]; struct passwd resbuf, *p; + int retval = 0; if (buf == NULL) { __set_errno (EINVAL); - return -1; + retval = -1; + goto error_out; } - buflen = __sysconf (_SC_GETPW_R_SIZE_MAX); - tmpbuf = alloca (buflen); - - if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0) - return -1; + if (__getpwuid_r (uid, &resbuf, tmpbuf, sizeof (tmpbuf), &p) != 0) + { + 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: + return retval; } weak_alias (__getpw, getpw)