BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t254011psql -h localhost -U postgresBuilt from patchset v5 (message #5), October 04, 2026 at 04:23 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t254011_5 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t254011_5 && git checkout t254011_5Patchset v5 (message #5) is on t254011_5
The following bug has been logged on the website:
Bug reference: 19730
Logged by: Shallow
Email address: theshallow27@gmail.com
PostgreSQL version: 18.6
Operating system: Linux
Description:
Summary
`pg_backup_start()` accepts a label containing a newline and embeds it
verbatim,
unescaped, into the line-oriented `backup_label` contents returned by
`pg_backup_stop()`. The extra line(s) shift the fields that follow `LABEL:`,
so
the file the documentation requires to be written "byte for byte without
modification" is malformed, and the server's own `read_backup_label()`
rejects
it with `FATAL` at restore time. The same function already backslash-escapes
`\n`/`\r` when building `tablespace_map`, but does nothing for the label.
Reproducing the Bug
```python
import db_harness
db_harness.query("""
CREATE OR REPLACE FUNCTION repro(lbl text) RETURNS text LANGUAGE plpgsql AS
$$
DECLARE r record;
BEGIN
PERFORM pg_backup_start(lbl, true);
SELECT * INTO r FROM pg_backup_stop(false);
RETURN r.labelfile;
END $$;
""")
label = "mybackup\nINCREMENTAL FROM LSN: 0/0"
r = db_harness.query("SELECT repro(%s)", [label])
assert r.ok, r.error
print(r.rows[0][0])
```
Output — the `labelfile` that the documentation says must be written
verbatim to
`<backup>/backup_label`:
```
START WAL LOCATION: 5/BC000028 (file 0000000100000005000000BC)
CHECKPOINT LOCATION: 5/BC000080
BACKUP METHOD: streamed
BACKUP FROM: primary
START TIME: 2026-09-26 08:46:09 UTC
LABEL: mybackup
INCREMENTAL FROM LSN: 0/0
START TIMELINE: 1
```
Restoring from that backup makes the server refuse to start:
```
FATAL: this is an incremental backup, not a data directory
HINT: Use pg_combinebackup to reconstruct a valid data directory.
```
With `label = "mybackup\nSTART TIMELINE: 0"` the same procedure yields:
```
FATAL: invalid data in file "backup_label"
DETAIL: Timeline ID parsed is 0, but expected 1.
```
Fix
Reject labels containing a newline or carriage return, alongside the
existing
length check. Escaping is not an option for the label without also changing
`read_backup_label()`, whose `%1023[^\n]` conversion does no de-escaping;
rejecting at `pg_backup_start()` time fails loudly and early, before a
useless
backup is taken.
```diff
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8861,6 +8861,17 @@ do_pg_backup_start(const char *backupidstr, bool
fast, List **tablespaces,
if (strlen(backupidstr) > MAXPGPATH)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d
bytes)",
MAXPGPATH)));+ /*
+ * The label is written as a single line of the backup_label file,
and read
+ * back with a conversion that stops at a newline and does no
de-escaping.
+ * An embedded newline would shift every field after "LABEL:" and
make the
+ * resulting backup_label unreadable at recovery time, so refuse it
here
+ * rather than producing an unrestorable backup.
+ */
+ if (strpbrk(backupidstr, "\n\r") != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("backup label must not contain
newline or carriage return characters")));
+
strlcpy(state->name, backupidstr, sizeof(state->name));
```
Hi,
Thanks for the report. I can reproduce it, but I do not see a real
use case for a newline inside a label.
A label that ends with a newline works fine. That is what a script
gets when it reads the label from a file. The file only breaks when
the newline is in the middle, with text on both sides. I cannot see
that happen by accident as well.
Do you mind sharing with us the use case of having a newline in
a backup file?
Thanks,
Shihao
On Thursday, October 1, 2026, shihao zhong <zhong950419@gmail.com> wrote:
Do you mind sharing with us the use case of having a newline in
a backup file?
The fix request seems like something we should just do barring foreseen
issues doing so. The system should not accept inputs it knows are
incompatible with the output it needs to produce. I agree there isn’t a
reason to try and make this usage actually work.
David J.
On Thu, Oct 01, 2026 at 08:25:05PM -0700, David G. Johnston wrote:
The fix request seems like something we should just do barring foreseen
issues doing so. The system should not accept inputs it knows are
incompatible with the output it needs to produce. I agree there isn’t a
reason to try and make this usage actually work.
I tend to agree with you here. There is no real use case in allowing
CR and LF characters in backup label names. If somebody has the idea
to abuse of that to introduce custom data into a label file, that's
probably a very bad idea anyway because it would overwrite the fields
a backend things are the good ones when producing the backup_label
file, leading to a most-probably broken instance.
In short, I'm on board with the addition of an extra check that
enforces this policy in do_pg_backup_start(), marking label files
coming from the SQL functions as much as the replication command
BASE_BACKUP.
Side note: v19 now disallows CRLFs in role, database and tablespace
names, see b380a56a3f95 and the reasons why. The same reasons do not
apply here, and are much lighter as a CRLF would only manipulate a
server to do an incorrect recovery. Like the other one, that's just
wrong.
Perhaps we should add one test query somewhere in a TAP script of
src/test/recovery/, while on it.
--
Michael
In short, I'm on board with the addition of an extra check that
enforces this policy in do_pg_backup_start()Perhaps we should add one test query somewhere in a TAP script of
src/test/recovery/, while on it.
Fair enough. Patch attached. It is the check from the report, with
the error wording of b380a56a3f95, and a test next to the "backup
label too long" one in 020_archive_status.pl.
One thing to note. A label that ends with a newline works today, and
this patch rejects it. A script that reads the label from a file can
hit that, so I think this should go to master only.
Thanks,
Shihao
On Thu, Oct 01, 2026 at 10:07:41PM -0600, shihao zhong wrote:
Fair enough. Patch attached. It is the check from the report, with
the error wording of b380a56a3f95, and a test next to the "backup
label too long" one in 020_archive_status.pl.
WFM.
One thing to note. A label that ends with a newline works today, and
this patch rejects it. A script that reads the label from a file can
hit that, so I think this should go to master only.
Of course, this is not something that would go into the stable
branches in any way.
Perhaps other will have some comments. Let's see.
--
Michael