Subject: Re: add non-option reordering to in-tree getopt_long

From: Nathan Bossart <[email-masked]>
Date: Wed, 21 Jun 2023 10:56:24 -0700

On Tue, Jun 20, 2023 at 02:12:44PM +0900, Kyotaro Horiguchi wrote:
> The argv elements get shuffled around many times with the
> patch. However, I couldn't find a way to decrease the count without
> resorting to a forward scan.  So I've concluded the current approach
> is them most effeicient, considering the complexity.

Yeah, I'm not sure it's worth doing anything more sophisticated.

> I tried some patterns with the patch and it generates the same results
> with the glibc version.
> 
> The TAP test looks fine and it catches the change.
> 
> Everything looks fine to me.

Thanks for reviewing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 7 Jul 2023 20:52:24 -0700

I spent some time tidying up the patch and adding a more detailed commit
message.

>From 2525e6aed066fe8eafdaab0d33c8c5df055c7e09 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Fri, 7 Jul 2023 20:00:47 -0700
Subject: [PATCH v5 1/1] Teach in-tree getopt_long() to move non-options to the
 end of argv.

Unlike the other implementations of getopt_long() I could find, the
in-tree implementation does not reorder non-options to the end of
argv.  Instead, it returns -1 as soon as the first non-option is
found, even if there are other options listed afterwards.  By
moving non-options to the end of argv, getopt_long() can parse all
specified options and return -1 when only non-options remain.
This quirk is periodically missed by hackers (e.g., 869aa40a27,
ffd398021c, and d9ddc50baf).

This commit introduces the aforementioned non-option reordering
behavior to the in-tree getopt_long() implementation.  The only
systems I'm aware of that use it are Windows and AIX, both of which
have been tested.

Special thanks to Noah Misch for his help validating behavior on
AIX.

Reviewed-by: Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20230609232257.GA121461%40nathanxps13
---
 src/bin/scripts/t/040_createuser.pl | 10 ++--
 src/port/getopt_long.c              | 79 ++++++++++++++++++++---------
 2 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 9ca282181d..3290e30c88 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -42,9 +42,8 @@ $node->issues_sql_like(
 	'add a role as a member with admin option of the newly created role');
 $node->issues_sql_like(
 	[
-		'createuser', '-m',
-		'regress_user3', '-m',
-		'regress user #4', 'REGRESS_USER5'
+		'createuser', 'REGRESS_USER5', '-m', 'regress_user3',
+		'-m', 'regress user #4'
 	],
 	qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/,
 	'add a role as a member of the newly created role');
@@ -73,11 +72,14 @@ $node->issues_sql_like(
 	qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
 	'--role');
 $node->issues_sql_like(
-	[ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
+	[ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
 	qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
 	'--member-of');
 
 $node->command_fails([ 'createuser', 'regress_user1' ],
 	'fails if role already exists');
+$node->command_fails(
+	[ 'createuser', 'regress_user1', '-m', 'regress_user2', 'regress_user3' ],
+	'fails for too many non-options');
 
 done_testing();
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index c989276988..3fc2cf36e9 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -50,8 +50,9 @@
  * This implementation does not use optreset.  Instead, we guarantee that
  * it can be restarted on a new argv array after a previous call returned -1,
  * if the caller resets optind to 1 before the first call of the new series.
- * (Internally, this means we must be sure to reset "place" to EMSG before
- * returning -1.)
+ *
+ * Note that this routine reorders the pointers in argv (despite the const
+ * qualifier) so that all non-options will be at the end when -1 is returned.
  */
 int
 getopt_long(int argc, char *const argv[],
@@ -60,40 +61,68 @@ getopt_long(int argc, char *const argv[],
 {
 	static char *place = EMSG;	/* option letter processing */
 	char	   *oli;			/* option letter list index */
+	static int	nonopt_start = -1;
+	static bool force_nonopt = false;
 
 	if (!*place)
 	{							/* update scanning pointer */
-		if (optind >= argc)
+		for (;;)
 		{
-			place = EMSG;
-			return -1;
-		}
+			char	  **args = (char **) argv;
 
-		place = argv[optind];
+			if (optind >= argc)
+			{
+				place = EMSG;
+				nonopt_start = -1;
+				force_nonopt = false;
+				return -1;
+			}
 
-		if (place[0] != '-')
-		{
-			place = EMSG;
-			return -1;
-		}
+			place = argv[optind];
+
+			/*
+			 * Any string that starts with '-' but is not equivalent to '-' or
+			 * '--' is considered an option.  If we see an option, we can
+			 * immediately break out of the loop since no argument reordering
+			 * is required.  Note that we treat '--' as the end of options and
+			 * immediately force reordering for every subsequent argument.
+			 * This reinstates the original order of all non-options (which
+			 * includes everything after the '--') before we return.
+			 */
+			if (!force_nonopt && place[0] == '-' && place[1])
+			{
+				if (place[1] != '-' || place[2])
+					break;
 
-		place++;
+				optind++;
+				force_nonopt = true;
+				continue;
+			}
 
-		if (!*place)
-		{
-			/* treat "-" as not being an option */
-			place = EMSG;
-			return -1;
-		}
+			/*
+			 * If only non-options remain, return -1.  Else, move the
+			 * non-option to the end and try again.
+			 */
+			if (optind == nonopt_start)
+			{
+				place = EMSG;
+				nonopt_start = -1;
+				force_nonopt = false;
+				return -1;
+			}
 
-		if (place[0] == '-' && place[1] == '\0')
-		{
-			/* found "--", treat it as end of options */
-			++optind;
-			place = EMSG;
-			return -1;
+			for (int i = optind; i < argc - 1; i++)
+				args[i] = args[i + 1];
+			args[argc - 1] = place;
+
+			if (nonopt_start == -1)
+				nonopt_start = argc - 1;
+			else
+				nonopt_start--;
 		}
 
+		place++;
+
 		if (place[0] == '-' && place[1])
 		{
 			/* long option */
-- 
2.25.1