From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject: Re: [PATCH] conf: Fix one Coverity CID 258163 warning, work around
 another one
Date: Tue, 24 May 2022 19:05:06 +1000
Message-ID: <YoyfwsxnZXUD7FcM@yekko>
In-Reply-To: <20220520090122.2510632-1-sbrivio@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5435171224473486735=="

--===============5435171224473486735==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 20, 2022 at 11:01:22AM +0200, Stefano Brivio wrote:
> In conf_runas(), Coverity reports that we might dereference uid and
> gid despite possibly being NULL (CWE-476) because of the check after
> the first sscanf(). They can't be NULL, but I actually wanted to
> check that UID and GID are non-zero (the user could otherwise pass
> --runas root:root and defy the whole mechanism).
>=20
> Later on, we have the same type of warning for 'gr': it's compared
> against NULL, so it might be NULL, which is actually the case: but
> in that case, we don't dereference it, because we'll return -ENOENT
> right away. Rewrite the clause to silence the warning.
>=20
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>

Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>

> ---
>  conf.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>=20
> diff --git a/conf.c b/conf.c
> index ddad9a3..d615cf5 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -857,7 +857,7 @@ static int conf_runas(const char *opt, unsigned int *ui=
d, unsigned int *gid)
>  	struct group *gr;
> =20
>  	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
> -	if (sscanf(opt, "%u:%u", uid, gid) =3D=3D 2 && uid && gid)
> +	if (sscanf(opt, "%u:%u", uid, gid) =3D=3D 2 && *uid && *gid)
>  		return 0;
> =20
>  	*uid =3D strtol(opt, &endptr, 0);
> @@ -874,12 +874,10 @@ static int conf_runas(const char *opt, unsigned int *=
uid, unsigned int *gid)
>  	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
>  	if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:"
>  			"%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) =3D=3D 2) {
> -		pw =3D getpwnam(ubuf);
> -		if (!pw || !(*uid =3D pw->pw_uid))
> +		if (!(pw =3D getpwnam(ubuf)) || !(*uid =3D pw->pw_uid))
>  			return -ENOENT;
> =20
> -		gr =3D getgrnam(gbuf);
> -		if (!gr || !(*gid =3D gr->gr_gid))
> +		if (!(gr =3D getgrnam(gbuf)) || !(*gid =3D gr->gr_gid))
>  			return -ENOENT;
> =20
>  		return 0;

--=20
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

--===============5435171224473486735==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz
MGRCK1h0Z3lwWTRnRXdZU0lGQW1LTW42d0FDZ2tRZ3lwWTRnRXcKWVNMOUVRLy9aNGpzU21oc05O
WEt3YjcxVVYyQTZERUdFWmc2THZ3RXUrY0FrRVIydE5iQXZoSnYyc3RkbnVVbgo2SkZLM29OZGFt
cDIwUjByZkx3cjhZRUs3eVJtTzJiVDUzWE00Y1NkaU8vYWNnTi9XNlVpazZCdXl4K01vZjVVCndU
a3BjMDJZQWdBYktZMk5iT2JtN1hwTjJ3cVFWb1llZUd3dzJCOEVBd01xbDMvdUZzdW1GQndoaVVJ
OUQvUEYKMzMydnRielF1anZiVlJVQkpjU0JjcEtWZGpyM1dkU2lodmlQbjFsZHlFUDdxRE9EZ0Vn
SDFNNC8zaXNvUDBsTQorUHZyVkJIWlRHbXowTmRDbmZ5Rkt1cndkNHBCSHl0dkluMUl0bnFiZW8y
d3Njd1Nyejc5UkJwa0NGNVovdHFwCm1FdkQzL0Fuekk0cmpCeTl6YkFZanNrdTVsTW0yV1Q0Y2tX
UGMzVDcvSTJHS2tGd1NIeHZ3cENvQTVqczZIOFAKdkUvRDMzWDZ5Q2ZndGQxM0RjeWNHVkZjekV1
d3JaRGMwNER0VC9aRExoUENEL2o2QzM5MUw1NWRxNERFZGU5bgp5RlNNTkpFVUdDcExnR2ZFR3FC
RGVsa3NGaGlUZUpseEF5VFByZWFtallQRkRJV2JTZFlJTkFMUW9TYWlNL1o2CkFSc1ZLOWdpT0Q0
Tnd0VzE2R1JHQjU4ajI2TERmL0YrMzVsNURtMHJyRTJUN0g4L2RTYnZDNEJ1cjVPVXBLMkIKSWhj
NHByUjMvZkRVMnAySmlpM0ZjcG44VGRDUnRKMHhtSDlMclFBbkIzbkhIbUEwWnZEOGVlNVFQVnpx
eHZ4VApoQTlDYzNySHZNbkhTakY3ZTJrSnZQQytCN0tMdFdKRWNSWG9acUhBdFc5NjRna0FyMkk9
Cj1lRUszCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============5435171224473486735==--