agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Nathan Bossart <nathandbossart@gmail.com>
To: Sehrope Sarkuni <sehrope@jackdb.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Fix getopt_long() argument handling and add tests
Date: Fri, 4 Sep 2026 16:58:11 -0500
Message-ID: <aps-8xDMYZfVQ5rQ@nathan> (raw)
In-Reply-To: <CAH7T-arxDuVCSkorO=k7+M-_JV0JFzMpN_EtKMyD2K0RDqZ2OA@mail.gmail.com>
References: <CAH7T-arxDuVCSkorO=k7+M-_JV0JFzMpN_EtKMyD2K0RDqZ2OA@mail.gmail.com>
On Sat, Aug 29, 2026 at 12:57:00PM -0400, Sehrope Sarkuni wrote:
> While reworking the pg_waldump TAP test [1] I ran "pg_waldump --stats
> --limit 5" and it failed on Windows CI with 'could not locate WAL file
> "5"'. For a long option with optional_argument and no "=", the port
> getopt_long() increments optind in the missing-argument branch and
> again at the end of the long option path, so the next option is
> skipped and its value becomes a positional argument. The same branch
> returns BADARG when optstring starts with a colon although nothing is
> missing.
>
> [...]
>
> 0003 fixes a second problem the tests turned up. The port moves each
> non-option to the end of argv as soon as it sees it, so by the time a
> following option looks for its argument, a non-option that came before
> it on the command line is already sitting after it and gets taken:
> "pg_amcheck mydb --jobs" parses as --jobs=mydb rather than reporting
> the missing argument. The system getopt_long() reports the missing
> argument. The fix bounds the argument search by the start of the
> moved block. 0004 adds the test cases for it, kept separate in case
> 0003 is backpatched without the test module.
Nice finds. I've attached what I have staged for commit. I've tried to
simplify the diffs a bit, and I also got rid of the new test module in
favor of adding cases to existing test files. I don't think we need to
bother back-patching 0001; the bug went undiscovered for ~23 years. I can
try back-patching 0002 to v17, though.
--
nathan
From f04a179b4f24f79f54ce81c6060f45c86a803c34 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 4 Sep 2026 16:37:01 -0500
Subject: [PATCH v2 1/2] Fix optional-argument handling in in-tree
getopt_long().
A long option with an optional argument that is given without "="
advances optind twice, so the following argument is skipped. For
example, "pg_waldump --stats --limit 5" complains that it cannot
locate WAL file "5". The same path also returns BADARG when
optstring starts with a colon, even though nothing is missing. To
fix, handle optional arguments before the missing-argument code,
which then only needs to deal with required arguments.
This is a bug fix and could be back-patched, but since this issue
went unnoticed for 23 years, I'm not going to bother.
Author: Sehrope Sarkuni <sehrope@jackdb.com>
Discussion: https://postgr.es/m/CAH7T-arxDuVCSkorO%3Dk7%2BM-_JV0JFzMpN_EtKMyD2K0RDqZ2OA%40mail.gmail.com
---
src/bin/pg_waldump/t/001_basic.pl | 3 +--
src/port/getopt_long.c | 11 +++++------
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl
index 8beac19eaff..7b33efc6299 100644
--- a/src/bin/pg_waldump/t/001_basic.pl
+++ b/src/bin/pg_waldump/t/001_basic.pl
@@ -340,11 +340,10 @@ sub test_pg_waldump
my ($stdout, $stderr);
my $result = IPC::Run::run [
- 'pg_waldump',
+ 'pg_waldump', @opts,
'--start' => $startlsn,
'--end' => $endlsn,
'--path' => $path,
- @opts
],
'>' => \$stdout,
'2>' => \$stderr;
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index 2e869fed58b..0a9a50189f1 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -137,8 +137,9 @@ retry:
{
if (place[namelen] == '=')
optarg = place + namelen + 1;
- else if (optind < argc - 1 &&
- has_arg == required_argument)
+ else if (has_arg == optional_argument)
+ optarg = NULL;
+ else if (optind < argc - 1)
{
optind++;
optarg = argv[optind];
@@ -152,16 +153,14 @@ retry:
return BADARG;
}
- if (opterr && has_arg == required_argument)
+ if (opterr)
fprintf(stderr,
"%s: option requires an argument -- %s\n",
argv[0], place);
place = EMSG;
- if (has_arg == required_argument)
- return BADCH;
- optarg = NULL;
+ return BADCH;
}
}
else
--
2.55.0
From b0fcd1a01b01e9e15d76dcadfae13f33f9ab8bb1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 4 Sep 2026 16:47:53 -0500
Subject: [PATCH v2 2/2] Fix option argument lookup in in-tree getopt_long().
The in-tree getopt_long() moves each non-option to the end of argv
as soon as it finds one, which puts a non-option that preceded an
option right where the option's argument lookup expects to find it.
For example, "vacuumdb postgres --jobs" takes "postgres" as the
number of jobs instead of complaining that --jobs is missing its
argument. To fix, stop the argument lookups at the start of the
moved non-options, which we already track to know when to stop
scanning.
Oversight in commit 411b720343.
Author: Sehrope Sarkuni <sehrope@jackdb.com>
Discussion: https://postgr.es/m/CAH7T-arxDuVCSkorO%3Dk7%2BM-_JV0JFzMpN_EtKMyD2K0RDqZ2OA%40mail.gmail.com
Backpatch-through: 17
---
src/bin/scripts/t/100_vacuumdb.pl | 4 ++++
src/port/getopt_long.c | 14 +++++++-------
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 7c4e35a6717..b78fa2a38de 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -240,6 +240,10 @@ $node->command_fails_like(
[ 'vacuumdb', '--all', 'postgres' ],
qr/cannot vacuum all databases and a specific one at the same time/,
'cannot use option --all and a dbname as argument at the same time');
+$node->command_fails_like(
+ [ 'vacuumdb', 'postgres', '--jobs' ],
+ qr/requires an argument/,
+ 'option missing its argument after a non-option');
$node->safe_psql(
'postgres', q|
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index 0a9a50189f1..f2edadb59d5 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -66,6 +66,9 @@ getopt_long(int argc, char *const argv[],
static int nonopt_start = -1;
static bool force_nonopt = false;
+ if (nonopt_start == -1)
+ nonopt_start = argc;
+
if (!*place)
{ /* update scanning pointer */
char **args = (char **) argv;
@@ -75,7 +78,7 @@ retry:
/*
* If we are out of arguments or only non-options remain, return -1.
*/
- if (optind >= argc || optind == nonopt_start)
+ if (optind >= nonopt_start)
{
place = EMSG;
nonopt_start = -1;
@@ -99,10 +102,7 @@ retry:
args[i] = args[i + 1];
args[argc - 1] = place;
- if (nonopt_start == -1)
- nonopt_start = argc - 1;
- else
- nonopt_start--;
+ nonopt_start--;
goto retry;
}
@@ -139,7 +139,7 @@ retry:
optarg = place + namelen + 1;
else if (has_arg == optional_argument)
optarg = NULL;
- else if (optind < argc - 1)
+ else if (optind < nonopt_start - 1)
{
optind++;
optarg = argv[optind];
@@ -222,7 +222,7 @@ retry:
{ /* need an argument */
if (*place) /* no white space */
optarg = place;
- else if (argc <= ++optind)
+ else if (nonopt_start <= ++optind)
{ /* no arg */
place = EMSG;
if (*optstring == ':')
--
2.55.0
Attachments:
[text/plain] v2-0001-Fix-optional-argument-handling-in-in-tree-getopt_.patch (2.4K, ../aps-8xDMYZfVQ5rQ@nathan/2-v2-0001-Fix-optional-argument-handling-in-in-tree-getopt_.patch)
download | inline diff:
From f04a179b4f24f79f54ce81c6060f45c86a803c34 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 4 Sep 2026 16:37:01 -0500
Subject: [PATCH v2 1/2] Fix optional-argument handling in in-tree
getopt_long().
A long option with an optional argument that is given without "="
advances optind twice, so the following argument is skipped. For
example, "pg_waldump --stats --limit 5" complains that it cannot
locate WAL file "5". The same path also returns BADARG when
optstring starts with a colon, even though nothing is missing. To
fix, handle optional arguments before the missing-argument code,
which then only needs to deal with required arguments.
This is a bug fix and could be back-patched, but since this issue
went unnoticed for 23 years, I'm not going to bother.
Author: Sehrope Sarkuni <sehrope@jackdb.com>
Discussion: https://postgr.es/m/CAH7T-arxDuVCSkorO%3Dk7%2BM-_JV0JFzMpN_EtKMyD2K0RDqZ2OA%40mail.gmail.com
---
src/bin/pg_waldump/t/001_basic.pl | 3 +--
src/port/getopt_long.c | 11 +++++------
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl
index 8beac19eaff..7b33efc6299 100644
--- a/src/bin/pg_waldump/t/001_basic.pl
+++ b/src/bin/pg_waldump/t/001_basic.pl
@@ -340,11 +340,10 @@ sub test_pg_waldump
my ($stdout, $stderr);
my $result = IPC::Run::run [
- 'pg_waldump',
+ 'pg_waldump', @opts,
'--start' => $startlsn,
'--end' => $endlsn,
'--path' => $path,
- @opts
],
'>' => \$stdout,
'2>' => \$stderr;
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index 2e869fed58b..0a9a50189f1 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -137,8 +137,9 @@ retry:
{
if (place[namelen] == '=')
optarg = place + namelen + 1;
- else if (optind < argc - 1 &&
- has_arg == required_argument)
+ else if (has_arg == optional_argument)
+ optarg = NULL;
+ else if (optind < argc - 1)
{
optind++;
optarg = argv[optind];
@@ -152,16 +153,14 @@ retry:
return BADARG;
}
- if (opterr && has_arg == required_argument)
+ if (opterr)
fprintf(stderr,
"%s: option requires an argument -- %s\n",
argv[0], place);
place = EMSG;
- if (has_arg == required_argument)
- return BADCH;
- optarg = NULL;
+ return BADCH;
}
}
else
--
2.55.0
[text/plain] v2-0002-Fix-option-argument-lookup-in-in-tree-getopt_long.patch (3.0K, ../aps-8xDMYZfVQ5rQ@nathan/3-v2-0002-Fix-option-argument-lookup-in-in-tree-getopt_long.patch)
download | inline diff:
From b0fcd1a01b01e9e15d76dcadfae13f33f9ab8bb1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 4 Sep 2026 16:47:53 -0500
Subject: [PATCH v2 2/2] Fix option argument lookup in in-tree getopt_long().
The in-tree getopt_long() moves each non-option to the end of argv
as soon as it finds one, which puts a non-option that preceded an
option right where the option's argument lookup expects to find it.
For example, "vacuumdb postgres --jobs" takes "postgres" as the
number of jobs instead of complaining that --jobs is missing its
argument. To fix, stop the argument lookups at the start of the
moved non-options, which we already track to know when to stop
scanning.
Oversight in commit 411b720343.
Author: Sehrope Sarkuni <sehrope@jackdb.com>
Discussion: https://postgr.es/m/CAH7T-arxDuVCSkorO%3Dk7%2BM-_JV0JFzMpN_EtKMyD2K0RDqZ2OA%40mail.gmail.com
Backpatch-through: 17
---
src/bin/scripts/t/100_vacuumdb.pl | 4 ++++
src/port/getopt_long.c | 14 +++++++-------
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 7c4e35a6717..b78fa2a38de 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -240,6 +240,10 @@ $node->command_fails_like(
[ 'vacuumdb', '--all', 'postgres' ],
qr/cannot vacuum all databases and a specific one at the same time/,
'cannot use option --all and a dbname as argument at the same time');
+$node->command_fails_like(
+ [ 'vacuumdb', 'postgres', '--jobs' ],
+ qr/requires an argument/,
+ 'option missing its argument after a non-option');
$node->safe_psql(
'postgres', q|
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index 0a9a50189f1..f2edadb59d5 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -66,6 +66,9 @@ getopt_long(int argc, char *const argv[],
static int nonopt_start = -1;
static bool force_nonopt = false;
+ if (nonopt_start == -1)
+ nonopt_start = argc;
+
if (!*place)
{ /* update scanning pointer */
char **args = (char **) argv;
@@ -75,7 +78,7 @@ retry:
/*
* If we are out of arguments or only non-options remain, return -1.
*/
- if (optind >= argc || optind == nonopt_start)
+ if (optind >= nonopt_start)
{
place = EMSG;
nonopt_start = -1;
@@ -99,10 +102,7 @@ retry:
args[i] = args[i + 1];
args[argc - 1] = place;
- if (nonopt_start == -1)
- nonopt_start = argc - 1;
- else
- nonopt_start--;
+ nonopt_start--;
goto retry;
}
@@ -139,7 +139,7 @@ retry:
optarg = place + namelen + 1;
else if (has_arg == optional_argument)
optarg = NULL;
- else if (optind < argc - 1)
+ else if (optind < nonopt_start - 1)
{
optind++;
optarg = argv[optind];
@@ -222,7 +222,7 @@ retry:
{ /* need an argument */
if (*place) /* no white space */
optarg = place;
- else if (argc <= ++optind)
+ else if (nonopt_start <= ++optind)
{ /* no arg */
place = EMSG;
if (*optstring == ':')
--
2.55.0
view thread (5+ messages) latest in thread
Message-ID: <aps-8xDMYZfVQ5rQ@nathan>
Permalink: ../aps-8xDMYZfVQ5rQ@nathan/
Also on: postgresql.org/message-id/aps-8xDMYZfVQ5rQ@nathan
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, sehrope@jackdb.com
Subject: Re: [PATCH] Fix getopt_long() argument handling and add tests
In-Reply-To: <aps-8xDMYZfVQ5rQ@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox