pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH] Fix missing argument handling in psql getopt
9+ messages / 2 participants
[nested] [flat]
* [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 10:06 Quentin Rameau <quinq@fifth.space>
0 siblings, 1 reply; 9+ messages in thread
From: Quentin Rameau @ 2019-08-25 10:06 UTC (permalink / raw)
To: pgsql-hackers
When passing an argument option with a missing argument, strcmp would
be called with the argv terminating NULL.
---
src/bin/psql/startup.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 4730c73396..cffbfc864e 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -667,12 +667,13 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
break;
case '?':
/* Actual help option given */
- if (strcmp(argv[optind - 1], "-?") == 0)
+ if (optind <= argc &&
+ strcmp(argv[optind - 1], "-?") == 0)
{
usage(NOPAGER);
exit(EXIT_SUCCESS);
}
- /* unknown option reported by getopt */
+ /* unknown option or missing argument */
else
goto unknown_option;
break;
--
2.23.0
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 15:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Quentin Rameau <quinq@fifth.space>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2019-08-25 15:02 UTC (permalink / raw)
To: Quentin Rameau <quinq@fifth.space>; +Cc: pgsql-hackers
Quentin Rameau <quinq@fifth.space> writes:
> When passing an argument option with a missing argument, strcmp would
> be called with the argv terminating NULL.
Um ... so how would control get there with optind too large?
What test case/platform are you considering?
(There really shouldn't be *any* case where getopt advances
optind past argc, imo.)
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 15:53 Quentin Rameau <quinq@fifth.space>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 9+ messages in thread
From: Quentin Rameau @ 2019-08-25 15:53 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <tgl@sss.pgh.pa.us>
Hello Tom,
> > When passing an argument option with a missing argument, strcmp
> > would be called with the argv terminating NULL.
>
> Um ... so how would control get there with optind too large?
That's from the getopt specification[0]:
“If 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.”
So in the case of “psql -h“, optarg points to argv[2] and optind is set
to 3 (optind is initialized to 1, then incremented by 2).
> What test case/platform are you considering?
Test case is just running psql -h.
The bug has been exposed by testing psql on musl.
> (There really shouldn't be *any* case where getopt advances
> optind past argc, imo.)
Actually that's the typical error case, as per the specification quoted
above.
[0] https://pubs.opengroup.org/onlinepubs/9699919799/functions/getopt.html
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 16:01 Quentin Rameau <quinq@fifth.space>
parent: Quentin Rameau <quinq@fifth.space>
1 sibling, 0 replies; 9+ messages in thread
From: Quentin Rameau @ 2019-08-25 16:01 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
> > Um ... so how would control get there with optind too large?
Sorry, I missed the simple explanation for that:
As option 'h' is missing an argument, getopt(_long) returns the
character '?' which is switched on through variable c, and the program
jumps to the '?' case handling.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 16:57 Tom Lane <tgl@sss.pgh.pa.us>
parent: Quentin Rameau <quinq@fifth.space>
1 sibling, 1 reply; 9+ messages in thread
From: Tom Lane @ 2019-08-25 16:57 UTC (permalink / raw)
To: Quentin Rameau <quinq@fifth.space>; +Cc: pgsql-hackers
Quentin Rameau <quinq@fifth.space> writes:
>> Um ... so how would control get there with optind too large?
> That's from the getopt specification[0]:
> “If 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.”
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.
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. 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.
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,
psql -f foo -?
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.
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 19:00 Quentin Rameau <quinq@fifth.space>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 9+ messages in thread
From: Quentin Rameau @ 2019-08-25 19:00 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <tgl@sss.pgh.pa.us>
> > “If 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.”
>
> 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:
“Otherwise, optarg shall point to the string following the option
character in that element of argv, and optind shall be incremented by 1.”
> 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.
>
> 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,
>
> psql -f foo -?
>
> 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.
Attachments:
[text/x-patch] 0001-Fix-handling-of-option.patch (959B, ../../20190825210009.586cfd5c@fifth.space/2-0001-Fix-handling-of-option.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 19:21 Quentin Rameau <quinq@fifth.space>
parent: Quentin Rameau <quinq@fifth.space>
1 sibling, 0 replies; 9+ messages in thread
From: Quentin Rameau @ 2019-08-25 19:21 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Tom Lane <tgl@sss.pgh.pa.us>
> 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.
Actually OpenBSD seems to set optopt to '?' by default, so the updated
attached patch ensure we start with an empty optopt.
Attachments:
[text/x-patch] 0001-Fix-handling-of-option.patch (1.2K, ../../20190825212136.04b56ac3@fifth.space/2-0001-Fix-handling-of-option.patch)
download | inline diff:
From ba0b331e67db4eb1a66ee7a1445a536f9788f0ad 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 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 4730c73396..333b49985a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -507,6 +507,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
int c;
memset(options, 0, sizeof *options);
+ optopt = 0;
while ((c = getopt_long(argc, argv, "aAbc:d:eEf:F:h:HlL:no:p:P:qR:sStT:U:v:VwWxXz?01",
long_options, &optindex)) != -1)
@@ -667,7 +668,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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 19:28 Tom Lane <tgl@sss.pgh.pa.us>
parent: Quentin Rameau <quinq@fifth.space>
1 sibling, 1 reply; 9+ messages in thread
From: Tom Lane @ 2019-08-25 19:28 UTC (permalink / raw)
To: Quentin Rameau <quinq@fifth.space>; +Cc: pgsql-hackers
Quentin Rameau <quinq@fifth.space> writes:
> 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.
Meh. We don't use optopt at all today, and I don't especially want
to start doing so. A patch that's trying to remove a platform
dependency (which is what this is, pedantic arguments that musl can
read POSIX better than anyone else notwithstanding) should not do
so by introducing hazards of new platform dependencies.
I've pushed your original patch (with some comment-tweaking).
It seems unlikely to break anything.
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Fix missing argument handling in psql getopt
@ 2019-08-25 19:45 Quentin Rameau <quinq@fifth.space>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 9+ messages in thread
From: Quentin Rameau @ 2019-08-25 19:45 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <tgl@sss.pgh.pa.us>
> I've pushed your original patch (with some comment-tweaking).
> It seems unlikely to break anything.
Thanks!
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2019-08-25 19:45 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-08-25 10:06 [PATCH] Fix missing argument handling in psql getopt Quentin Rameau <quinq@fifth.space>
2019-08-25 15:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-08-25 15:53 ` Quentin Rameau <quinq@fifth.space>
2019-08-25 16:01 ` Quentin Rameau <quinq@fifth.space>
2019-08-25 16:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-08-25 19:00 ` Quentin Rameau <quinq@fifth.space>
2019-08-25 19:21 ` Quentin Rameau <quinq@fifth.space>
2019-08-25 19:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-08-25 19:45 ` Quentin Rameau <quinq@fifth.space>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox