Re: correction suggestion for https://www.postgresql.org/docs/17/auth-username-maps.html - Mailing list pgsql-docs

I wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> I didn't add an example but felt the point "be referenced a single time
>> within" to be needed since, usefulness not withstanding, writing \1\1 for
>> database-username works but only the first instance of \1 is replaced.

> Hmm, I wonder if that isn't a bug we should fix.  It's hard to believe
> anyone is relying on the second \1 *not* getting replaced, and perhaps
> there are use-cases for multiple replacements.

Here's a quick patch for that.  I hacked up 003_peer.pl enough to
prove that multiple replacement works, but that test change is not
committable as-is because it assumes that the "system user" name
is "postgres".  I don't like the existing test much either, since it
only tests the case of the substituted string being empty, which
means the substitution code could be quite broken and it wouldn't
notice.  But I don't offhand see a way to improve that without
making assumptions about the incoming name...

            regards, tom lane

diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 332fad27835..fecee8224d0 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -2873,8 +2873,11 @@ check_ident_usermap(IdentLine *identLine, const char *usermap_name,
             !token_has_regexp(identLine->pg_user) &&
             (ofs = strstr(identLine->pg_user->string, "\\1")) != NULL)
         {
+            const char *repl_str;
+            size_t        repl_len;
+            char       *old_pg_user;
             char       *expanded_pg_user;
-            int            offset;
+            size_t        offset;

             /* substitution of the first argument requested */
             if (matches[1].rm_so < 0)
@@ -2886,18 +2889,33 @@ check_ident_usermap(IdentLine *identLine, const char *usermap_name,
                 *error_p = true;
                 return;
             }
+            repl_str = system_user + matches[1].rm_so;
+            repl_len = matches[1].rm_eo - matches[1].rm_so;

             /*
-             * length: original length minus length of \1 plus length of match
-             * plus null terminator
+             * It's allowed to have more than one \1 in the string, and we'll
+             * replace them all.  But that's pretty unusual so we optimize on
+             * the assumption of only one occurrence, which motivates doing
+             * repeated replacements instead of making two passes over the
+             * string to determine the final length right away.
              */
-            expanded_pg_user = palloc0(strlen(identLine->pg_user->string) - 2 + (matches[1].rm_eo - matches[1].rm_so)
+1); 
-            offset = ofs - identLine->pg_user->string;
-            memcpy(expanded_pg_user, identLine->pg_user->string, offset);
-            memcpy(expanded_pg_user + offset,
-                   system_user + matches[1].rm_so,
-                   matches[1].rm_eo - matches[1].rm_so);
-            strcat(expanded_pg_user, ofs + 2);
+            old_pg_user = identLine->pg_user->string;
+            do
+            {
+                /*
+                 * length: current length minus length of \1 plus length of
+                 * replacement plus null terminator
+                 */
+                expanded_pg_user = palloc(strlen(old_pg_user) - 2 + repl_len + 1);
+                /* ofs points into the old_pg_user string at this point */
+                offset = ofs - old_pg_user;
+                memcpy(expanded_pg_user, old_pg_user, offset);
+                memcpy(expanded_pg_user + offset, repl_str, repl_len);
+                strcpy(expanded_pg_user + offset + repl_len, ofs + 2);
+                if (old_pg_user != identLine->pg_user->string)
+                    pfree(old_pg_user);
+                old_pg_user = expanded_pg_user;
+            } while ((ofs = strstr(old_pg_user + offset + repl_len, "\\1")) != NULL);

             /*
              * Mark the token as quoted, so it will only be compared literally
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
index f2320b62c87..8a9431e5594 100644
--- a/src/test/authentication/t/003_peer.pl
+++ b/src/test/authentication/t/003_peer.pl
@@ -93,6 +93,8 @@ if ($node->log_contains(
 $node->safe_psql('postgres', qq{CREATE ROLE testmapuser LOGIN});
 $node->safe_psql('postgres', "CREATE ROLE testmapgroup NOLOGIN");
 $node->safe_psql('postgres', "GRANT testmapgroup TO testmapuser");
+# This role is for testing \1 substitution.
+$node->safe_psql('postgres', qq{CREATE ROLE testgresgresmapuser LOGIN});
 # Note the double quotes here.
 $node->safe_psql('postgres', 'CREATE ROLE "testmapgroupliteral\\1" LOGIN');
 $node->safe_psql('postgres', 'GRANT "testmapgroupliteral\\1" TO testmapuser');
@@ -212,10 +214,10 @@ test_role(

 # Success as the regular expression matches and \1 is replaced in the given
 # subexpression.
-reset_pg_ident($node, 'mypeermap', qq{/^$system_user(.*)\$}, 'test\1mapuser');
+reset_pg_ident($node, 'mypeermap', qq{/^post(.*)\$}, 'test\1\1mapuser');
 test_role(
     $node,
-    qq{testmapuser},
+    qq{testgresgresmapuser},
     'peer',
     0,
     'with regular expression in user name map with \1 replaced',