* [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam @ 2017-06-06 16:21 Rudolph Pereira 2017-06-09 0:00 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Rudolph Pereira @ 2017-06-06 16:21 UTC (permalink / raw) To: musl [-- Attachment #1.1: Type: text/plain, Size: 465 bytes --] Hi, currently, the getgr, getpw, and getspnam functions in musl return ERANGE when the allocated buffer is not large enough, but do not set errno to the same value. This causes issues with utilities, for example the "shadow" utilities (useradd/mod, groupmod, etc.) which assume this behaviour (which at least gnu libc exhibits) and leads to groups having a small limit on the number of members. The attached patch, against 1.1.16, corrects this. Cheers, Rudolph [-- Attachment #1.2: Type: text/html, Size: 579 bytes --] [-- Attachment #2: 1100-getX-errno.patch --] [-- Type: text/x-patch, Size: 1120 bytes --] --- musl-1.1.16.orig/src/passwd/getgr_r.c +++ musl-1.1.16/src/passwd/getgr_r.c @@ -1,5 +1,6 @@ #include "pwf.h" #include <pthread.h> +#include <errno.h> #define FIX(x) (gr->gr_##x = gr->gr_##x-line+buf) @@ -19,6 +20,7 @@ if (*res && size < len + (nmem+1)*sizeof(char *) + 32) { *res = 0; rv = ERANGE; + errno = ERANGE; } if (*res) { buf += (16-(uintptr_t)buf)%16; --- musl-1.1.16.orig/src/passwd/getpw_r.c +++ musl-1.1.16/src/passwd/getpw_r.c @@ -1,5 +1,6 @@ #include "pwf.h" #include <pthread.h> +#include <errno.h> #define FIX(x) (pw->pw_##x = pw->pw_##x-line+buf) @@ -16,6 +17,7 @@ if (*res && size < len) { *res = 0; rv = ERANGE; + errno = ERANGE; } if (*res) { memcpy(buf, line, len); --- musl-1.1.16.orig/src/passwd/getspnam_r.c +++ musl-1.1.16/src/passwd/getspnam_r.c @@ -3,6 +3,7 @@ #include <sys/stat.h> #include <ctype.h> #include <pthread.h> +#include <errno.h> #include "pwf.h" /* This implementation support Openwall-style TCB passwords in place of @@ -104,6 +105,7 @@ } if (buf[k-1] != '\n') { rv = ERANGE; + errno = ERANGE; break; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam 2017-06-06 16:21 [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam Rudolph Pereira @ 2017-06-09 0:00 ` Rich Felker 2017-06-11 16:59 ` Rudolph Pereira 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2017-06-09 0:00 UTC (permalink / raw) To: musl On Tue, Jun 06, 2017 at 12:21:39PM -0400, Rudolph Pereira wrote: > Hi, > > currently, the getgr, getpw, and getspnam functions in musl return ERANGE > when the allocated buffer is not large enough, but do not set errno to the > same value. This causes issues with utilities, for example the "shadow" > utilities (useradd/mod, groupmod, etc.) which assume this behaviour (which > at least gnu libc exhibits) and leads to groups having a small limit on the > number of members. > > The attached patch, against 1.1.16, corrects this. > > Cheers, > Rudolph > --- musl-1.1.16.orig/src/passwd/getgr_r.c > +++ musl-1.1.16/src/passwd/getgr_r.c > @@ -1,5 +1,6 @@ > #include "pwf.h" > #include <pthread.h> > +#include <errno.h> > > #define FIX(x) (gr->gr_##x = gr->gr_##x-line+buf) > > @@ -19,6 +20,7 @@ > if (*res && size < len + (nmem+1)*sizeof(char *) + 32) { > *res = 0; > rv = ERANGE; > + errno = ERANGE; > } > if (*res) { > buf += (16-(uintptr_t)buf)%16; > --- musl-1.1.16.orig/src/passwd/getpw_r.c > +++ musl-1.1.16/src/passwd/getpw_r.c > @@ -1,5 +1,6 @@ > #include "pwf.h" > #include <pthread.h> > +#include <errno.h> > > #define FIX(x) (pw->pw_##x = pw->pw_##x-line+buf) > > @@ -16,6 +17,7 @@ > if (*res && size < len) { > *res = 0; > rv = ERANGE; > + errno = ERANGE; > } > if (*res) { > memcpy(buf, line, len); > --- musl-1.1.16.orig/src/passwd/getspnam_r.c > +++ musl-1.1.16/src/passwd/getspnam_r.c > @@ -3,6 +3,7 @@ > #include <sys/stat.h> > #include <ctype.h> > #include <pthread.h> > +#include <errno.h> > #include "pwf.h" > > /* This implementation support Openwall-style TCB passwords in place of > @@ -104,6 +105,7 @@ > } > if (buf[k-1] != '\n') { > rv = ERANGE; > + errno = ERANGE; > break; > } I don't think this patch is complete. A nonzero value of rv can also come from __getpw_a/__getgr_a. Insted, just before return there should probably be: if (rv) errno = rv; Does that look correct? I'm not sure about getspnam_r; it might actually be missing some error cases right now. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam 2017-06-09 0:00 ` Rich Felker @ 2017-06-11 16:59 ` Rudolph Pereira 2017-06-15 0:05 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Rudolph Pereira @ 2017-06-11 16:59 UTC (permalink / raw) To: musl [-- Attachment #1.1: Type: text/plain, Size: 2812 bytes --] Hi Rich, thanks for the feedback. I've attached a patch that implements errno setting as you suggested, other than a couple of cases where the code immediately returns. This also brings it in line with existing code (in __getpw_a/__getgr_a) so makes things more consistent. Please see attached - note this is against HEAD. On 8 June 2017 at 20:00, Rich Felker <dalias@libc.org> wrote: > On Tue, Jun 06, 2017 at 12:21:39PM -0400, Rudolph Pereira wrote: > > Hi, > > > > currently, the getgr, getpw, and getspnam functions in musl return ERANGE > > when the allocated buffer is not large enough, but do not set errno to > the > > same value. This causes issues with utilities, for example the "shadow" > > utilities (useradd/mod, groupmod, etc.) which assume this behaviour > (which > > at least gnu libc exhibits) and leads to groups having a small limit on > the > > number of members. > > > > The attached patch, against 1.1.16, corrects this. > > > > Cheers, > > Rudolph > > > --- musl-1.1.16.orig/src/passwd/getgr_r.c > > +++ musl-1.1.16/src/passwd/getgr_r.c > > @@ -1,5 +1,6 @@ > > #include "pwf.h" > > #include <pthread.h> > > +#include <errno.h> > > > > #define FIX(x) (gr->gr_##x = gr->gr_##x-line+buf) > > > > @@ -19,6 +20,7 @@ > > if (*res && size < len + (nmem+1)*sizeof(char *) + 32) { > > *res = 0; > > rv = ERANGE; > > + errno = ERANGE; > > } > > if (*res) { > > buf += (16-(uintptr_t)buf)%16; > > --- musl-1.1.16.orig/src/passwd/getpw_r.c > > +++ musl-1.1.16/src/passwd/getpw_r.c > > @@ -1,5 +1,6 @@ > > #include "pwf.h" > > #include <pthread.h> > > +#include <errno.h> > > > > #define FIX(x) (pw->pw_##x = pw->pw_##x-line+buf) > > > > @@ -16,6 +17,7 @@ > > if (*res && size < len) { > > *res = 0; > > rv = ERANGE; > > + errno = ERANGE; > > } > > if (*res) { > > memcpy(buf, line, len); > > --- musl-1.1.16.orig/src/passwd/getspnam_r.c > > +++ musl-1.1.16/src/passwd/getspnam_r.c > > @@ -3,6 +3,7 @@ > > #include <sys/stat.h> > > #include <ctype.h> > > #include <pthread.h> > > +#include <errno.h> > > #include "pwf.h" > > > > /* This implementation support Openwall-style TCB passwords in place of > > @@ -104,6 +105,7 @@ > > } > > if (buf[k-1] != '\n') { > > rv = ERANGE; > > + errno = ERANGE; > > break; > > } > > I don't think this patch is complete. A nonzero value of rv can also > come from __getpw_a/__getgr_a. Insted, just before return there should > probably be: > > if (rv) errno = rv; > > Does that look correct? I'm not sure about getspnam_r; it might > actually be missing some error cases right now. > > Rich > [-- Attachment #1.2: Type: text/html, Size: 3984 bytes --] [-- Attachment #2: getX-errno-v2.patch --] [-- Type: text/x-patch, Size: 1317 bytes --] diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c index e8cc811..0c87ab0 100644 --- a/src/passwd/getpw_r.c +++ b/src/passwd/getpw_r.c @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si } free(line); pthread_setcancelstate(cs, 0); + if (rv) errno = rv; return rv; } diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c index 9233952..47ce3d3 100644 --- a/src/passwd/getspnam_r.c +++ b/src/passwd/getspnam_r.c @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct /* Disallow potentially-malicious user names */ if (*name=='.' || strchr(name, '/') || !l) + { + errno = EINVAL; return EINVAL; + } /* Buffer size must at least be able to hold name, plus some.. */ - if (size < l+100) return ERANGE; + if (size < l+100) + { + errno = EINVAL; + return ERANGE; + } /* Protect against truncation */ if (snprintf(path, sizeof path, "/etc/tcb/%s/shadow", name) >= sizeof path) + { + errno = EINVAL; return EINVAL; + } fd = open(path, O_RDONLY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC); if (fd >= 0) { @@ -112,5 +122,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct break; } pthread_cleanup_pop(1); + if (rv) errno = rv; return rv; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam 2017-06-11 16:59 ` Rudolph Pereira @ 2017-06-15 0:05 ` Rich Felker 2017-06-15 12:18 ` Rudolph Pereira 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2017-06-15 0:05 UTC (permalink / raw) To: musl On Sun, Jun 11, 2017 at 12:59:51PM -0400, Rudolph Pereira wrote: > Hi Rich, > > thanks for the feedback. I've attached a patch that implements errno > setting as you suggested, other than a couple of cases where the code > immediately returns. This also brings it in line with existing code > (in __getpw_a/__getgr_a) so makes things more consistent. Please see > attached - note this is against HEAD. It looks like you omitted the change to getgr_r.c corresponding to this one for getpw_r.c: > diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c > index e8cc811..0c87ab0 100644 > --- a/src/passwd/getpw_r.c > +++ b/src/passwd/getpw_r.c > @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si > } > free(line); > pthread_setcancelstate(cs, 0); > + if (rv) errno = rv; > return rv; > } Also: > diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c > index 9233952..47ce3d3 100644 > --- a/src/passwd/getspnam_r.c > +++ b/src/passwd/getspnam_r.c > @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct > > /* Disallow potentially-malicious user names */ > if (*name=='.' || strchr(name, '/') || !l) > + { > + errno = EINVAL; > return EINVAL; > + } Please use consistent style for braces (open brace on if line). Alternatively (if you don't balk at the style; not sure if others will like it), this works: - return EINVAL; + return errno = EINVAL; Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam 2017-06-15 0:05 ` Rich Felker @ 2017-06-15 12:18 ` Rudolph Pereira 2017-06-15 17:04 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Rudolph Pereira @ 2017-06-15 12:18 UTC (permalink / raw) To: musl [-- Attachment #1.1: Type: text/plain, Size: 1888 bytes --] Hi, please see the updated attached patch. In terms of style, I've implemented your latter suggestion as it is more compact, but happy either way. Cheers, Rudolph On 14 June 2017 at 20:05, Rich Felker <dalias@libc.org> wrote: > On Sun, Jun 11, 2017 at 12:59:51PM -0400, Rudolph Pereira wrote: > > Hi Rich, > > > > thanks for the feedback. I've attached a patch that implements errno > > setting as you suggested, other than a couple of cases where the code > > immediately returns. This also brings it in line with existing code > > (in __getpw_a/__getgr_a) so makes things more consistent. Please see > > attached - note this is against HEAD. > > It looks like you omitted the change to getgr_r.c corresponding to > this one for getpw_r.c: > > > diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c > > index e8cc811..0c87ab0 100644 > > --- a/src/passwd/getpw_r.c > > +++ b/src/passwd/getpw_r.c > > @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct > passwd *pw, char *buf, si > > } > > free(line); > > pthread_setcancelstate(cs, 0); > > + if (rv) errno = rv; > > return rv; > > } > > Also: > > > diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c > > index 9233952..47ce3d3 100644 > > --- a/src/passwd/getspnam_r.c > > +++ b/src/passwd/getspnam_r.c > > @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp, > char *buf, size_t size, struct > > > > /* Disallow potentially-malicious user names */ > > if (*name=='.' || strchr(name, '/') || !l) > > + { > > + errno = EINVAL; > > return EINVAL; > > + } > > Please use consistent style for braces (open brace on if line). > Alternatively (if you don't balk at the style; not sure if others will > like it), this works: > > - return EINVAL; > + return errno = EINVAL; > > Rich > [-- Attachment #1.2: Type: text/html, Size: 2583 bytes --] [-- Attachment #2: getX-errno-v3.patch --] [-- Type: text/x-patch, Size: 1638 bytes --] diff --git a/src/passwd/getgr_r.c b/src/passwd/getgr_r.c index 7246e8a..f3e8f60 100644 --- a/src/passwd/getgr_r.c +++ b/src/passwd/getgr_r.c @@ -34,6 +34,7 @@ static int getgr_r(const char *name, gid_t gid, struct group *gr, char *buf, siz free(mem); free(line); pthread_setcancelstate(cs, 0); + if (rv) errno = rv; return rv; } diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c index e8cc811..0c87ab0 100644 --- a/src/passwd/getpw_r.c +++ b/src/passwd/getpw_r.c @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si } free(line); pthread_setcancelstate(cs, 0); + if (rv) errno = rv; return rv; } diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c index 9233952..e488b67 100644 --- a/src/passwd/getspnam_r.c +++ b/src/passwd/getspnam_r.c @@ -72,14 +72,15 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct /* Disallow potentially-malicious user names */ if (*name=='.' || strchr(name, '/') || !l) - return EINVAL; + return errno = EINVAL; /* Buffer size must at least be able to hold name, plus some.. */ - if (size < l+100) return ERANGE; + if (size < l+100) + return errno = EINVAL; /* Protect against truncation */ if (snprintf(path, sizeof path, "/etc/tcb/%s/shadow", name) >= sizeof path) - return EINVAL; + return errno = EINVAL; fd = open(path, O_RDONLY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC); if (fd >= 0) { @@ -112,5 +113,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct break; } pthread_cleanup_pop(1); + if (rv) errno = rv; return rv; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam 2017-06-15 12:18 ` Rudolph Pereira @ 2017-06-15 17:04 ` Rich Felker 0 siblings, 0 replies; 6+ messages in thread From: Rich Felker @ 2017-06-15 17:04 UTC (permalink / raw) To: musl On Thu, Jun 15, 2017 at 08:18:18AM -0400, Rudolph Pereira wrote: > Hi, > > please see the updated attached patch. In terms of style, I've implemented > your latter suggestion as it is more compact, but happy either way. Thanks. Committing it. Rich > On 14 June 2017 at 20:05, Rich Felker <dalias@libc.org> wrote: > > > On Sun, Jun 11, 2017 at 12:59:51PM -0400, Rudolph Pereira wrote: > > > Hi Rich, > > > > > > thanks for the feedback. I've attached a patch that implements errno > > > setting as you suggested, other than a couple of cases where the code > > > immediately returns. This also brings it in line with existing code > > > (in __getpw_a/__getgr_a) so makes things more consistent. Please see > > > attached - note this is against HEAD. > > > > It looks like you omitted the change to getgr_r.c corresponding to > > this one for getpw_r.c: > > > > > diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c > > > index e8cc811..0c87ab0 100644 > > > --- a/src/passwd/getpw_r.c > > > +++ b/src/passwd/getpw_r.c > > > @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct > > passwd *pw, char *buf, si > > > } > > > free(line); > > > pthread_setcancelstate(cs, 0); > > > + if (rv) errno = rv; > > > return rv; > > > } > > > > Also: > > > > > diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c > > > index 9233952..47ce3d3 100644 > > > --- a/src/passwd/getspnam_r.c > > > +++ b/src/passwd/getspnam_r.c > > > @@ -72,14 +72,24 @@ int getspnam_r(const char *name, struct spwd *sp, > > char *buf, size_t size, struct > > > > > > /* Disallow potentially-malicious user names */ > > > if (*name=='.' || strchr(name, '/') || !l) > > > + { > > > + errno = EINVAL; > > > return EINVAL; > > > + } > > > > Please use consistent style for braces (open brace on if line). > > Alternatively (if you don't balk at the style; not sure if others will > > like it), this works: > > > > - return EINVAL; > > + return errno = EINVAL; > > > > Rich > > > diff --git a/src/passwd/getgr_r.c b/src/passwd/getgr_r.c > index 7246e8a..f3e8f60 100644 > --- a/src/passwd/getgr_r.c > +++ b/src/passwd/getgr_r.c > @@ -34,6 +34,7 @@ static int getgr_r(const char *name, gid_t gid, struct group *gr, char *buf, siz > free(mem); > free(line); > pthread_setcancelstate(cs, 0); > + if (rv) errno = rv; > return rv; > } > > diff --git a/src/passwd/getpw_r.c b/src/passwd/getpw_r.c > index e8cc811..0c87ab0 100644 > --- a/src/passwd/getpw_r.c > +++ b/src/passwd/getpw_r.c > @@ -27,6 +27,7 @@ static int getpw_r(const char *name, uid_t uid, struct passwd *pw, char *buf, si > } > free(line); > pthread_setcancelstate(cs, 0); > + if (rv) errno = rv; > return rv; > } > > diff --git a/src/passwd/getspnam_r.c b/src/passwd/getspnam_r.c > index 9233952..e488b67 100644 > --- a/src/passwd/getspnam_r.c > +++ b/src/passwd/getspnam_r.c > @@ -72,14 +72,15 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct > > /* Disallow potentially-malicious user names */ > if (*name=='.' || strchr(name, '/') || !l) > - return EINVAL; > + return errno = EINVAL; > > /* Buffer size must at least be able to hold name, plus some.. */ > - if (size < l+100) return ERANGE; > + if (size < l+100) > + return errno = EINVAL; > > /* Protect against truncation */ > if (snprintf(path, sizeof path, "/etc/tcb/%s/shadow", name) >= sizeof path) > - return EINVAL; > + return errno = EINVAL; > > fd = open(path, O_RDONLY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC); > if (fd >= 0) { > @@ -112,5 +113,6 @@ int getspnam_r(const char *name, struct spwd *sp, char *buf, size_t size, struct > break; > } > pthread_cleanup_pop(1); > + if (rv) errno = rv; > return rv; > } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-15 17:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-06 16:21 [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam Rudolph Pereira 2017-06-09 0:00 ` Rich Felker 2017-06-11 16:59 ` Rudolph Pereira 2017-06-15 0:05 ` Rich Felker 2017-06-15 12:18 ` Rudolph Pereira 2017-06-15 17:04 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ 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 NNTP newsgroup(s).