DomainKey-Signature: a=rsa-sha1; c=nofws; d=fifth.space; h=date:from:to
	:cc:subject:message-id:in-reply-to:references:mime-version
	:content-type; q=dns; s=dkim; b=VZR5acctGfnqOdgE4FSuyT3h2apD9Y/6
	rH5LiY/tuLJVJEKcByQ4lfSgCdwdCNntLePLxTacQa5acKIgXZ2rUIDt28hgoDMn
	yEyfU4k/b4w6OGddz35TkTZ9CHSa4GizHPaSm+emFw0p61q4PQEy5OH12Wd5keaT
	dSFxrDlfVAh0JOUfhnCxICnWJs0z4WDCKkSj5GDf1BnvDHnE1/lgxT3AHGT6qErt
	a1y40AJ1BnHzRdqExMwTVkEpeXRfw1wZDHB4ARh4TPWTLsLg5XLDV2HN618f2ZNw
	xkyrNIuZeC6mhoUDr4fgJmwJy31oln6Q855nmgAJFuvQIr6ImZV2HA==
Date: Sun, 25 Aug 2019 21:00:09 +0200
From: Quentin Rameau <quinq@fifth.space>
To: pgsql-hackers@postgresql.org
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: [PATCH] Fix missing argument handling in psql getopt
Message-ID: <20190825210009.586cfd5c@fifth.space>
In-Reply-To: <7842.1566752268@sss.pgh.pa.us>
References: <20190825100617.GA6087@fifth.space>
	<14042.1566745352@sss.pgh.pa.us>
	<20190825175338.74ed0f0e@fifth.space>
	<7842.1566752268@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/_eGHSN7B9ucS1dXH.MiKkUY"
Precedence: bulk

--MP_/_eGHSN7B9ucS1dXH.MiKkUY
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

> > =E2=80=9CIf the option was the last character in the string pointed to =
by an
> > element of argv, then optarg shall contain the next element of argv,
> > and optind shall be incremented by 2. If the resulting value of optind
> > is greater than argc, this indicates a missing option-argument, and
> > getopt() shall return an error indication.=E2=80=9D =20
>=20
> Hm, interesting --- glibc doesn't seem to do that (advance optind past
> argc), nor do any of the principal BSDen.  I see that this could be
> read as requiring it, but it seems like musl is pretty out of step
> by reading it that way.

Yes they don't, but the specification looks pretty clear to me, there's
nothing ambiguous about it.
Now it's a matter of implementation willing to adhere or not to it.

> I actually don't care for that code very much and would prefer that
> we nuke it entirely, because I think it's assuming more than it ought to
> about the meaning of optind: in the case of multiple option letters in one
> argv element, it's unspecified exactly when optind advances.

Yes, optind shouldn't be used this way here.
But it's specified exactly how and when optind advances, following the
above code, which specifies the first use case of separated option and
argument, there's a second case for grouped option and argument:

=E2=80=9COtherwise, optarg shall point to the string following the option
character in that element of argv, and optind shall be incremented by 1.=E2=
=80=9D

> So the other
> problem here is that sometimes it's looking at the argv element *before*
> the relevant one.  (It's easily demonstrated that this is so with glibc's
> getopt().)  Probably that doesn't ever result in wrong behavior in
> practice, but it still seems bogus.
>=20
> The normal case of "psql -?" is handled before we ever get to this code,
> so if we just deleted '?' entirely from this logic, it would mostly do
> what we want.  The case that would change is, eg,
>=20
> 	psql -f foo -?
>=20
> where now you get a usage message but you'd just get an "invalid option"
> complaint without the special case.  Seeing that none of our other
> command-line programs have this special case, I'm not sure why psql
> still does.

Another better way, I think, to fix this is to check for optopt
instead, which would be set to the option which caused the error, which
if empty means there isn't an error.

Patch attached.

--MP_/_eGHSN7B9ucS1dXH.MiKkUY
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename=0001-Fix-handling-of-option.patch

From 8aaec7d151567385c43d8966f46f0e5e4837c93c Mon Sep 17 00:00:00 2001
From: Quentin Rameau <quinq@fifth.space>
Date: Sun, 25 Aug 2019 20:45:29 +0200
Subject: [PATCH] Fix handling of ? option

Using optind to check back the original given option this way is bogus
and could lead to dereferencing argv out of bounds with a missing
argument to an option.

The proper way to verify if an error has occured is to check if optopt
has been set.
---
 src/bin/psql/startup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 4730c73396..e5f6894177 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -667,7 +667,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
 				break;
 			case '?':
 				/* Actual help option given */
-				if (strcmp(argv[optind - 1], "-?") == 0)
+				if (!optopt)
 				{
 					usage(NOPAGER);
 					exit(EXIT_SUCCESS);
-- 
2.23.0


--MP_/_eGHSN7B9ucS1dXH.MiKkUY--