From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11485 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: Thu, 15 Jun 2017 08:18:18 -0400 Message-ID: References: <20170609000030.GM1627@brightrain.aerifal.cx> <20170615000518.GK1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="001a11498f00357ca00551feab2b" X-Trace: blaine.gmane.org 1497529132 23920 195.159.176.226 (15 Jun 2017 12:18:52 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 15 Jun 2017 12:18:52 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-11498-gllmg-musl=m.gmane.org@lists.openwall.com Thu Jun 15 14:18:49 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 1dLTjc-00060T-Pd for gllmg-musl@m.gmane.org; Thu, 15 Jun 2017 14:18:48 +0200 Original-Received: (qmail 20292 invoked by uid 550); 15 Jun 2017 12:18:52 -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 20255 invoked from network); 15 Jun 2017 12:18:51 -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=bM1E5fEJVPsGHSdiATVKXV2cBkaEgRWGVo4elEUxjC0=; b=C9mfKUAmW5Zwk9Itpg2sstdfwfw3cUCfBSB505N8nmT568UqRLMEjqg9IDqVlf06+l V6liPCrbTjJNyenfGUjkMHjQagmPKoBaE0uVTBYQiyE3wpELcGIxa4G86W+0Dwzyg+ZD PFuTO9BHNJV7vN0TYYBInz6sLKSQqC0RdHc2BV+eS9jLj+PLgPfJ3ohq0PHirRoHIhBk lQ4cdUoR7vaNC/Xpg30mvqZkIFlEwEbxKZjknzz/x+8H22nMM3GeFHSWk0FuULEcs+41 LanodKA+d7S++GkHvN2WrFDqpGLrcszlP5KkvGLlhcIf7KhG6H18Ap83UU88bzxIPRrK T9fA== 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=bM1E5fEJVPsGHSdiATVKXV2cBkaEgRWGVo4elEUxjC0=; b=YXN8XElv5YqjN7Mo0mG+TcJscn6qB6e3+v+HPStir6lJjbr7JpFRhS9T918G1i4u7d hliPq3O2s/4lME1U3KvX8RJVgKUNizMkTkcWvJsOxn+GD5nwmZOSJoP4wMK9PIBUqqPq 0iLr58yO3zX4qDyed7XZljjQdWToTW6G67Md5ND1wC5p+FqXxr9yK3HG2A7f6DJmYQrv UOrbwXQFmkQRjeIbucjsnI41AQ1X2Hl2rAXfnLinpbHAHvNgLT8/nUSLyFsS0p9r6Sqm xHJkmOAT8ZS4kjbq3o5FEej7GdERK/EGYFLrpHWGxr4KmK0Fu6aA7HWl17gwQhEeqen/ Zy+g== X-Gm-Message-State: AKS2vOxelO7LDFMVUnH209iy0pBy0Sm5JkeS0hTK8QWKnaJDZ54lQZCq vm7ROffjhh3Fr4Lyz7D20+KpBVOxaLMjwwE= X-Received: by 10.36.214.7 with SMTP id o7mr4821354itg.53.1497529119491; Thu, 15 Jun 2017 05:18:39 -0700 (PDT) In-Reply-To: <20170615000518.GK1627@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:11485 Archived-At: --001a11498f00357ca00551feab2b Content-Type: multipart/alternative; boundary="001a11498f00357c9c0551feab29" --001a11498f00357c9c0551feab29 Content-Type: text/plain; charset="UTF-8" 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 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 > --001a11498f00357c9c0551feab29 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

please see the updated attached pat= ch. 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 err= no
> setting as you suggested, other than a couple of cases where the code<= br> > 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, stru= ct passwd *pw, char *buf, si
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0free(line);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_setcancelstate(cs, 0);
> +=C2=A0 =C2=A0 =C2=A0if (rv) errno =3D rv;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return rv;
>=C2=A0 }

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
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Disallow potentially-malicious user names= */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (*name=3D=3D'.' || strchr(name, &= #39;/') || !l)
> +=C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0errno =3D EINVAL;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return EINVAL; > +=C2=A0 =C2=A0 =C2=A0}

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<= br> like it), this works:

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return EINVAL;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return errno =3D EI= NVAL;

Rich

--001a11498f00357c9c0551feab29-- --001a11498f00357ca00551feab2b Content-Type: text/x-patch; charset="US-ASCII"; name="getX-errno-v3.patch" Content-Disposition: attachment; filename="getX-errno-v3.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_j3ye4i190 ZGlmZiAtLWdpdCBhL3NyYy9wYXNzd2QvZ2V0Z3Jfci5jIGIvc3JjL3Bhc3N3ZC9nZXRncl9yLmMK aW5kZXggNzI0NmU4YS4uZjNlOGY2MCAxMDA2NDQKLS0tIGEvc3JjL3Bhc3N3ZC9nZXRncl9yLmMK KysrIGIvc3JjL3Bhc3N3ZC9nZXRncl9yLmMKQEAgLTM0LDYgKzM0LDcgQEAgc3RhdGljIGludCBn ZXRncl9yKGNvbnN0IGNoYXIgKm5hbWUsIGdpZF90IGdpZCwgc3RydWN0IGdyb3VwICpnciwgY2hh ciAqYnVmLCBzaXoKICAJZnJlZShtZW0pOwogIAlmcmVlKGxpbmUpOwogCXB0aHJlYWRfc2V0Y2Fu Y2Vsc3RhdGUoY3MsIDApOworCWlmIChydikgZXJybm8gPSBydjsKIAlyZXR1cm4gcnY7CiB9CiAK ZGlmZiAtLWdpdCBhL3NyYy9wYXNzd2QvZ2V0cHdfci5jIGIvc3JjL3Bhc3N3ZC9nZXRwd19yLmMK aW5kZXggZThjYzgxMS4uMGM4N2FiMCAxMDA2NDQKLS0tIGEvc3JjL3Bhc3N3ZC9nZXRwd19yLmMK KysrIGIvc3JjL3Bhc3N3ZC9nZXRwd19yLmMKQEAgLTI3LDYgKzI3LDcgQEAgc3RhdGljIGludCBn ZXRwd19yKGNvbnN0IGNoYXIgKm5hbWUsIHVpZF90IHVpZCwgc3RydWN0IHBhc3N3ZCAqcHcsIGNo YXIgKmJ1Ziwgc2kKIAl9CiAgCWZyZWUobGluZSk7CiAJcHRocmVhZF9zZXRjYW5jZWxzdGF0ZShj cywgMCk7CisJaWYgKHJ2KSBlcnJubyA9IHJ2OwogCXJldHVybiBydjsKIH0KIApkaWZmIC0tZ2l0 IGEvc3JjL3Bhc3N3ZC9nZXRzcG5hbV9yLmMgYi9zcmMvcGFzc3dkL2dldHNwbmFtX3IuYwppbmRl eCA5MjMzOTUyLi5lNDg4YjY3IDEwMDY0NAotLS0gYS9zcmMvcGFzc3dkL2dldHNwbmFtX3IuYwor KysgYi9zcmMvcGFzc3dkL2dldHNwbmFtX3IuYwpAQCAtNzIsMTQgKzcyLDE1IEBAIGludCBnZXRz cG5hbV9yKGNvbnN0IGNoYXIgKm5hbWUsIHN0cnVjdCBzcHdkICpzcCwgY2hhciAqYnVmLCBzaXpl X3Qgc2l6ZSwgc3RydWN0CiAKIAkvKiBEaXNhbGxvdyBwb3RlbnRpYWxseS1tYWxpY2lvdXMgdXNl ciBuYW1lcyAqLwogCWlmICgqbmFtZT09Jy4nIHx8IHN0cmNocihuYW1lLCAnLycpIHx8ICFsKQot CQlyZXR1cm4gRUlOVkFMOworCQlyZXR1cm4gZXJybm8gPSBFSU5WQUw7CiAKIAkvKiBCdWZmZXIg c2l6ZSBtdXN0IGF0IGxlYXN0IGJlIGFibGUgdG8gaG9sZCBuYW1lLCBwbHVzIHNvbWUuLiAqLwot CWlmIChzaXplIDwgbCsxMDApIHJldHVybiBFUkFOR0U7CisJaWYgKHNpemUgPCBsKzEwMCkKKwkJ cmV0dXJuIGVycm5vID0gRUlOVkFMOwogCiAJLyogUHJvdGVjdCBhZ2FpbnN0IHRydW5jYXRpb24g Ki8KIAlpZiAoc25wcmludGYocGF0aCwgc2l6ZW9mIHBhdGgsICIvZXRjL3RjYi8lcy9zaGFkb3ci LCBuYW1lKSA+PSBzaXplb2YgcGF0aCkKLQkJcmV0dXJuIEVJTlZBTDsKKwkJcmV0dXJuIGVycm5v ID0gRUlOVkFMOwogCiAJZmQgPSBvcGVuKHBhdGgsIE9fUkRPTkxZfE9fTk9GT0xMT1d8T19OT05C TE9DS3xPX0NMT0VYRUMpOwogCWlmIChmZCA+PSAwKSB7CkBAIC0xMTIsNSArMTEzLDYgQEAgaW50 IGdldHNwbmFtX3IoY29uc3QgY2hhciAqbmFtZSwgc3RydWN0IHNwd2QgKnNwLCBjaGFyICpidWYs IHNpemVfdCBzaXplLCBzdHJ1Y3QKIAkJYnJlYWs7CiAJfQogCXB0aHJlYWRfY2xlYW51cF9wb3Ao MSk7CisJaWYgKHJ2KSBlcnJubyA9IHJ2OwogCXJldHVybiBydjsKIH0K --001a11498f00357ca00551feab2b--