Subject: fairywren is generating bogus BASE_BACKUP commands

From: Robert Haas <[email-masked]>
Date: Fri, 21 Jan 2022 16:42:24 -0500

Thomas Munro pointed out this failure to me on fairywren:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2022-01-21%2020%3A10%3A22

He theorizes that I need some perl2host magic in there, which may well
be true. But I also noticed this:

# Running: pg_basebackup --no-sync -cfast --target
server:/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_basebackup/tmp_check/tmp_test_Ag8r/backuponserver
-X none
pg_basebackup: error: could not initiate base backup: ERROR:
unrecognized target: "server;C"

"server" is a valid backup target, but "server;C" is not. And I think
this must be a bug on the client side, because the server logs the
generated query:

2022-01-21 20:53:11.618 UTC [8404:10] 010_pg_basebackup.pl LOG:
received replication command: BASE_BACKUP ( LABEL 'pg_basebackup base
backup',  PROGRESS,  CHECKPOINT 'fast',  MANIFEST 'yes',
TABLESPACE_MAP,  TARGET 'server;C',  TARGET_DETAIL
'\\tools\\msys64\\home\\pgrunner\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\tmp_check\\tmp_test_Ag8r\\backuponserver')

So it's not that the server parses the query and introduces gibberish
-- the client has already introduced gibberish when constructing the
query. But the code to do that is pretty straightforward -- we just
call strchr to find the colon in the backup target, and then
pnstrdup() the part before the colon and use the latter part as-is. If
pnstrdup were failing to add a terminating \0 then this would be quite
plausible, but I think it shouldn't. Unless the operating sytem's
strnlen() is buggy? That seems like a stretch, so feel free to tell me
what obvious stupid thing I did here and am not seeing...

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Fri, 21 Jan 2022 17:09:19 -0500

> what obvious stupid thing I did here and am not seeing...

I think the backup_target string was already corrupted that way when
pg_basebackup absorbed it from optarg.  It's pretty hard to believe that
the strchr/pnstrdup stanza got it wrong.  However, comparing the
TARGET_DETAIL to what the TAP test says it issued:

# Running: pg_basebackup --no-sync -cfast --target server:/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_basebackup/tmp_check/tmp_test_Ag8r/backuponserver -X none
pg_basebackup: error: could not initiate base backup: ERROR:  unrecognized target: "server;C"

it's absolutely clear that something decided to munge the target string.
Given that colon is reserved in Windows filename syntax, it's not
so surprising if it munged it wrong, or at least more aggressively
than you expected.

I kinda wonder if this notation for the target was well-chosen.
Keeping the file name strictly separate from the "type" keyword
might be a wiser move.  Quite aside from Windows-isms, there
are going to be usages where this is hard to tell from a URL.
(If memory serves, double leading slash is significant to some
networked file systems.)

While we're on the subject of ill-chosen option syntax: "-cfast"
with non double dashes?  Really?  That's horribly ambiguous.
Most programs would parse something like that as five single-letter
options, and most users who know Unix would expect it to mean that.

			regards, tom lane