From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11446 Path: news.gmane.org!.POSTED!not-for-mail From: Rudolph Pereira Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix errno not being set to ERANGE by getgr, getpw, and getspnam Date: Sun, 11 Jun 2017 12:59:51 -0400 Message-ID: References: <20170609000030.GM1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="001a11452f1cb0db080551b222f8" X-Trace: blaine.gmane.org 1497200433 27319 195.159.176.226 (11 Jun 2017 17:00:33 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 11 Jun 2017 17:00:33 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-11459-gllmg-musl=m.gmane.org@lists.openwall.com Sun Jun 11 19:00:29 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1dK6E1-0006ml-0T for gllmg-musl@m.gmane.org; Sun, 11 Jun 2017 19:00:29 +0200 Original-Received: (qmail 16356 invoked by uid 550); 11 Jun 2017 17:00:31 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 16330 invoked from network); 11 Jun 2017 17:00:30 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=occamsec-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=Ny4i7FlA0yKeXkl+Rde+rKnBMAyp4X+AfX8AVtvzB0s=; b=zmWAMIALMDAokS00jW2jgqOPSipwiLNh6K6ywfGZFE0syrVTP0VaJzRQ/frj4WrACR iA/O7ibitgLLaX/Tm7lxrnNUO2YxYE+2qGjlQfdGOXM+w7b/eDvHg0Gbv6MK0MDQzCS9 yOVIyBQwenMn/kSvO8t0m4Y48UaUXt1gF5q1xAAqKdcnRzLcFNYinsad/Jg7YEZwEtZn Ik3omzBheMypzUFLyT6hR7LIImXCqB6CWl/vgXABFkUp+U4MuBWeKFNMki3Sg+lKSCK4 hKSXr7XQKCs6MvrhbTIadr0xp+1jNuLuq0d1UIyyP3EjQDs6Pqj/WlS8Q+JKA2c0MsAP ou7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=Ny4i7FlA0yKeXkl+Rde+rKnBMAyp4X+AfX8AVtvzB0s=; b=GymfUGPxQBtb++rocVIZ5JbczX3wtnVlgjQNQ/h+WyKNaV1IoIvWeWoJDaNtFMJXU2 23D8xNo9a39Sk3rxvGtMvCFx7g4dEhwSYrhSRb3vxRh6QtsX5IoJdPqPo19ZebwH30u7 zvWHyFvgGmZ9mtw1jd0gVTVeOKjQXLlpXMpqJaCz2kHD+pZRKY9cjbQpbJqWhljtOkEc VneQhgX79EioESjf42gG7alkjzUBRTnc2OWUOYvn9b7WLTqb2duBv1Wn+p+V/xrpzUyX csg1CSXTZGxPvxsJYRuUU6zcgdGJ905vcmVvusVD3GYEi4a5Gn9sUpZoKdVfEbxR6GAL lJzA== X-Gm-Message-State: AODbwcBv2sIRmC5yXilWlVLkGyYrdUS8J7ZQlnFeWgAlyadEIMFS923x YkvVNceIf/LM4vXe8UIydQkQwya+jYeVsTRaLg== X-Received: by 10.107.176.140 with SMTP id z134mr33432775ioe.76.1497200411574; Sun, 11 Jun 2017 10:00:11 -0700 (PDT) In-Reply-To: <20170609000030.GM1627@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:11446 Archived-At: --001a11452f1cb0db080551b222f8 Content-Type: multipart/alternative; boundary="001a11452f1cb0db030551b222f6" --001a11452f1cb0db030551b222f6 Content-Type: text/plain; charset="UTF-8" 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 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 > > +#include > > > > #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 > > +#include > > > > #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 > > #include > > #include > > +#include > > #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 > --001a11452f1cb0db030551b222f6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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=C2=A0__getpw_a/__getgr_a) so makes thing= s more consistent. Please see attached - note this is against HEAD.

On 8 June 201= 7 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 ERA= NGE
> 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 "s= hadow"
> utilities (useradd/mod, groupmod, etc.) which assume this behaviour (w= hich
> at least gnu libc exhibits) and leads to groups having a small limit o= n 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 @@
>=C2=A0 #include "pwf.h"
>=C2=A0 #include <pthread.h>
> +#include <errno.h>
>
>=C2=A0 #define FIX(x) (gr->gr_##x =3D gr->gr_##x-line+buf)
>
> @@ -19,6 +20,7 @@
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (*res && size < len + (nmem+1)= *sizeof(char *) + 32) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*res =3D 0;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rv =3D ERANGE; > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0errno =3D ERANGE;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (*res) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0buf +=3D (16-(ui= ntptr_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 @@
>=C2=A0 #include "pwf.h"
>=C2=A0 #include <pthread.h>
> +#include <errno.h>
>
>=C2=A0 #define FIX(x) (pw->pw_##x =3D pw->pw_##x-line+buf)
>
> @@ -16,6 +17,7 @@
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (*res && size < len) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*res =3D 0;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rv =3D ERANGE; > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0errno =3D ERANGE;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (*res) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memcpy(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 @@
>=C2=A0 #include <sys/stat.h>
>=C2=A0 #include <ctype.h>
>=C2=A0 #include <pthread.h>
> +#include <errno.h>
>=C2=A0 #include "pwf.h"
>
>=C2=A0 /* This implementation support Openwall-style TCB passwords in p= lace of
> @@ -104,6 +105,7 @@
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (buf[k-1] != =3D '\n') {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0rv =3D ERANGE;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0errno =3D ERANGE;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0break;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

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:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (rv) errno =3D rv;

Does that look correct? I'm not sure about getspnam_r; it might
actually be missing some error cases right now.

Rich

--001a11452f1cb0db030551b222f6-- --001a11452f1cb0db080551b222f8 Content-Type: text/x-patch; charset="US-ASCII"; name="getX-errno-v2.patch" Content-Disposition: attachment; filename="getX-errno-v2.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_j3syeoqu0 ZGlmZiAtLWdpdCBhL3NyYy9wYXNzd2QvZ2V0cHdfci5jIGIvc3JjL3Bhc3N3ZC9nZXRwd19yLmMK aW5kZXggZThjYzgxMS4uMGM4N2FiMCAxMDA2NDQKLS0tIGEvc3JjL3Bhc3N3ZC9nZXRwd19yLmMK KysrIGIvc3JjL3Bhc3N3ZC9nZXRwd19yLmMKQEAgLTI3LDYgKzI3LDcgQEAgc3RhdGljIGludCBn ZXRwd19yKGNvbnN0IGNoYXIgKm5hbWUsIHVpZF90IHVpZCwgc3RydWN0IHBhc3N3ZCAqcHcsIGNo YXIgKmJ1Ziwgc2kKIAl9CiAgCWZyZWUobGluZSk7CiAJcHRocmVhZF9zZXRjYW5jZWxzdGF0ZShj cywgMCk7CisJaWYgKHJ2KSBlcnJubyA9IHJ2OwogCXJldHVybiBydjsKIH0KIApkaWZmIC0tZ2l0 IGEvc3JjL3Bhc3N3ZC9nZXRzcG5hbV9yLmMgYi9zcmMvcGFzc3dkL2dldHNwbmFtX3IuYwppbmRl eCA5MjMzOTUyLi40N2NlM2QzIDEwMDY0NAotLS0gYS9zcmMvcGFzc3dkL2dldHNwbmFtX3IuYwor KysgYi9zcmMvcGFzc3dkL2dldHNwbmFtX3IuYwpAQCAtNzIsMTQgKzcyLDI0IEBAIGludCBnZXRz cG5hbV9yKGNvbnN0IGNoYXIgKm5hbWUsIHN0cnVjdCBzcHdkICpzcCwgY2hhciAqYnVmLCBzaXpl X3Qgc2l6ZSwgc3RydWN0CiAKIAkvKiBEaXNhbGxvdyBwb3RlbnRpYWxseS1tYWxpY2lvdXMgdXNl ciBuYW1lcyAqLwogCWlmICgqbmFtZT09Jy4nIHx8IHN0cmNocihuYW1lLCAnLycpIHx8ICFsKQor CXsKKwkJZXJybm8gPSBFSU5WQUw7CiAJCXJldHVybiBFSU5WQUw7CisJfQogCiAJLyogQnVmZmVy IHNpemUgbXVzdCBhdCBsZWFzdCBiZSBhYmxlIHRvIGhvbGQgbmFtZSwgcGx1cyBzb21lLi4gKi8K LQlpZiAoc2l6ZSA8IGwrMTAwKSByZXR1cm4gRVJBTkdFOworCWlmIChzaXplIDwgbCsxMDApCisJ eworCQllcnJubyA9IEVJTlZBTDsKKwkJcmV0dXJuIEVSQU5HRTsKKwl9CiAKIAkvKiBQcm90ZWN0 IGFnYWluc3QgdHJ1bmNhdGlvbiAqLwogCWlmIChzbnByaW50ZihwYXRoLCBzaXplb2YgcGF0aCwg Ii9ldGMvdGNiLyVzL3NoYWRvdyIsIG5hbWUpID49IHNpemVvZiBwYXRoKQorCXsKKwkJZXJybm8g PSBFSU5WQUw7CiAJCXJldHVybiBFSU5WQUw7CisJfQogCiAJZmQgPSBvcGVuKHBhdGgsIE9fUkRP TkxZfE9fTk9GT0xMT1d8T19OT05CTE9DS3xPX0NMT0VYRUMpOwogCWlmIChmZCA+PSAwKSB7CkBA IC0xMTIsNSArMTIyLDYgQEAgaW50IGdldHNwbmFtX3IoY29uc3QgY2hhciAqbmFtZSwgc3RydWN0 IHNwd2QgKnNwLCBjaGFyICpidWYsIHNpemVfdCBzaXplLCBzdHJ1Y3QKIAkJYnJlYWs7CiAJfQog CXB0aHJlYWRfY2xlYW51cF9wb3AoMSk7CisJaWYgKHJ2KSBlcnJubyA9IHJ2OwogCXJldHVybiBy djsKIH0K --001a11452f1cb0db080551b222f8--