agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedBUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
6+ messages / 4 participants
[nested] [flat]
* BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
@ 2026-09-29 19:15 PG Bug reporting form <noreply@postgresql.org>
2026-10-02 03:17 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: PG Bug reporting form @ 2026-09-29 19:15 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: theshallow27@gmail.com
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));
```
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
2026-09-29 19:15 BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup PG Bug reporting form <noreply@postgresql.org>
@ 2026-10-02 03:17 ` shihao zhong <zhong950419@gmail.com>
2026-10-02 03:25 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup David G. Johnston <david.g.johnston@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: shihao zhong @ 2026-10-02 03:17 UTC (permalink / raw)
To: theshallow27@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
2026-09-29 19:15 BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup PG Bug reporting form <noreply@postgresql.org>
2026-10-02 03:17 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
@ 2026-10-02 03:25 ` David G. Johnston <david.g.johnston@gmail.com>
2026-10-02 03:49 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: David G. Johnston @ 2026-10-02 03:25 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: theshallow27@gmail.com <theshallow27@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
2026-09-29 19:15 BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup PG Bug reporting form <noreply@postgresql.org>
2026-10-02 03:17 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
2026-10-02 03:25 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup David G. Johnston <david.g.johnston@gmail.com>
@ 2026-10-02 03:49 ` Michael Paquier <michael@paquier.xyz>
2026-10-02 04:07 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2026-10-02 03:49 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: shihao zhong <zhong950419@gmail.com>; theshallow27@gmail.com <theshallow27@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ar8px1yG51ESobdm@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
2026-09-29 19:15 BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup PG Bug reporting form <noreply@postgresql.org>
2026-10-02 03:17 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
2026-10-02 03:25 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup David G. Johnston <david.g.johnston@gmail.com>
2026-10-02 03:49 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup Michael Paquier <michael@paquier.xyz>
@ 2026-10-02 04:07 ` shihao zhong <zhong950419@gmail.com>
2026-10-02 04:32 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: shihao zhong @ 2026-10-02 04:07 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; theshallow27@gmail.com <theshallow27@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> 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
Attachments:
[application/octet-stream] v1-0001-Reject-CR-and-LF-in-backup-labels.patch (2.4K, ../../CAGRkXqS7znpXT6P-iLnTZthnMm1EtUx+N8yZ-WSkU3i38vi1fg@mail.gmail.com/3-v1-0001-Reject-CR-and-LF-in-backup-labels.patch)
download | inline diff:
From 505537d0435c7ba97917a084f184079a95058871 Mon Sep 17 00:00:00 2001
From: Shihao <zhong950419@gmail.com>
Date: Thu, 1 Oct 2026 22:03:02 -0600
Subject: [PATCH v1] Reject CR and LF in backup labels
The label is written as one line of the backup_label file. A label
with a newline added extra lines to the file, and recovery could read
those as other fields or fail on them. Reject such labels in
do_pg_backup_start(), which covers both pg_backup_start() and
BASE_BACKUP.
Bug: #19730
Reported-by: Shallow <theshallow27@gmail.com>
Discussion: https://postgr.es/m/19730-85a6044c9e72774a@postgresql.org
---
src/backend/access/transam/xlog.c | 6 ++++++
src/test/recovery/t/020_archive_status.pl | 13 +++++++++++++
2 files changed, 19 insertions(+)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 9ec0be77ca0..b90ce916457 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9995,6 +9995,12 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
+ /* The label is stored as a single line of the backup_label file. */
+ if (strpbrk(backupidstr, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("backup label contains a newline or carriage return character")));
+
strlcpy(state->name, backupidstr, sizeof(state->name));
/*
diff --git a/src/test/recovery/t/020_archive_status.pl b/src/test/recovery/t/020_archive_status.pl
index 5bb8aa9ec17..94d49751f7c 100644
--- a/src/test/recovery/t/020_archive_status.pl
+++ b/src/test/recovery/t/020_archive_status.pl
@@ -260,6 +260,19 @@ $cmdret = $primary->psql(
stderr => \$stderr);
is($cmdret, 3, "psql fails correctly");
like($stderr, qr/backup label too long/, "pg_backup_start fails gracefully");
+
+# Newlines and carriage returns are not allowed in backup labels
+foreach my $label ("E'one\\nbackup'", "E'one\\rbackup'")
+{
+ $primary->psql(
+ 'postgres',
+ "SELECT pg_backup_start($label)",
+ stderr => \$stderr);
+ like(
+ $stderr,
+ qr/backup label contains a newline or carriage return character/,
+ "pg_backup_start rejects label $label");
+}
$primary->safe_psql('postgres',
"SELECT pg_backup_start('onebackup'); SELECT pg_backup_stop();");
$primary->safe_psql('postgres', "SELECT pg_backup_start('twobackup')");
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup
2026-09-29 19:15 BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup PG Bug reporting form <noreply@postgresql.org>
2026-10-02 03:17 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
2026-10-02 03:25 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup David G. Johnston <david.g.johnston@gmail.com>
2026-10-02 03:49 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup Michael Paquier <michael@paquier.xyz>
2026-10-02 04:07 ` Re: BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup shihao zhong <zhong950419@gmail.com>
@ 2026-10-02 04:32 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2026-10-02 04:32 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; theshallow27@gmail.com <theshallow27@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ar8z1ClV9rWfAxVo@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-10-02 04:32 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-29 19:15 BUG #19730: PostgreSQL `pg_backup_start` accepts newlines in the backup label, producing an unrestorable `backup PG Bug reporting form <noreply@postgresql.org>
2026-10-02 03:17 ` shihao zhong <zhong950419@gmail.com>
2026-10-02 03:25 ` David G. Johnston <david.g.johnston@gmail.com>
2026-10-02 03:49 ` Michael Paquier <michael@paquier.xyz>
2026-10-02 04:07 ` shihao zhong <zhong950419@gmail.com>
2026-10-02 04:32 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox