From bc353e73aa64e3fc950ab6eb68271c0e90b925b6 Mon Sep 17 00:00:00 2001 From: Andrey Rachitskiy Date: Fri, 14 Aug 2026 18:21:08 +0500 Subject: [PATCH v2] Fix SEGV in ReadToc on NULL ReadStr results ReadStr() may return NULL for a negative length word. That is valid for nullable TOC fields and the dependency-list terminator, but ReadToc() passed such pointers to sscanf()/strcmp() for fields that are always present in a valid archive. Add ReadRequiredStr() for those sites so pg_restore reports a corrupt TOC instead of crashing. Add a regression test in src/bin/pg_dump/t/001_basic.pl that builds minimal custom-format archives, each with one required TOC field encoded as a NULL string, and checks that pg_restore reports a corrupt TOC for every ReadRequiredStr() call site. Bug: #19613 Author: Andrey Rachitskiy Reported-by: Ilia Kashintsev --- src/bin/pg_dump/pg_backup_archiver.c | 30 ++++++++++-- src/bin/pg_dump/t/001_basic.pl | 69 ++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index d7da3fc4325..8b5ec260b86 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -85,6 +85,7 @@ static int _discoverArchiveFormat(ArchiveHandle *AH); static int RestoringToDB(ArchiveHandle *AH); static void dump_lo_buf(ArchiveHandle *AH); +static char *ReadRequiredStr(ArchiveHandle *AH, const char *fieldname); static void dumpTimestamp(ArchiveHandle *AH, const char *msg, time_t tim); static void SetOutput(ArchiveHandle *AH, const char *filename, const pg_compress_specification compression_spec); @@ -2227,6 +2228,25 @@ ReadStr(ArchiveHandle *AH) return buf; } +/* + * Like ReadStr(), but require a non-NULL string. + * + * A negative length is a valid NULL encoding in the archive format + * (nullable TOC fields, dependency-list terminator). Use this only for + * fields that a valid dump always writes as a real string. Getting NULL + * there means the TOC is incomplete or corrupt. + */ +static char * +ReadRequiredStr(ArchiveHandle *AH, const char *fieldname) +{ + char *result; + + result = ReadStr(AH); + if (result == NULL) + pg_fatal("missing %s in TOC -- perhaps a corrupt TOC", fieldname); + return result; +} + static bool _fileExistsInDirectory(const char *dir, const char *filename) { @@ -2735,18 +2755,18 @@ ReadToc(ArchiveHandle *AH) if (AH->version >= K_VERS_1_8) { - tmp = ReadStr(AH); + tmp = ReadRequiredStr(AH, "table OID"); sscanf(tmp, "%u", &te->catalogId.tableoid); free(tmp); } else te->catalogId.tableoid = InvalidOid; - tmp = ReadStr(AH); + tmp = ReadRequiredStr(AH, "OID"); sscanf(tmp, "%u", &te->catalogId.oid); free(tmp); - te->tag = ReadStr(AH); - te->desc = ReadStr(AH); + te->tag = ReadRequiredStr(AH, "entry tag"); + te->desc = ReadRequiredStr(AH, "entry description"); if (AH->version >= K_VERS_1_11) { @@ -2802,7 +2822,7 @@ ReadToc(ArchiveHandle *AH) is_supported = false; else { - tmp = ReadStr(AH); + tmp = ReadRequiredStr(AH, "WITH OIDS marker"); if (strcmp(tmp, "true") == 0) is_supported = false; diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl index b2558046224..840a1313376 100644 --- a/src/bin/pg_dump/t/001_basic.pl +++ b/src/bin/pg_dump/t/001_basic.pl @@ -250,4 +250,73 @@ command_fails_like( 'pg_dumpall: option --exclude-database cannot be used together with -g/--globals-only' ); +######################################### +# Corrupt archive checks + +# ReadToc() reads several TOC fields that a valid dump always stores as a real +# string. A NULL there (encoded as a negative length) used to reach sscanf() +# or strcmp() and crash pg_restore (bug #19613). Build minimal custom-format +# archives, each with one such field encoded as NULL, and check that pg_restore +# reports a corrupt TOC instead of crashing. + +# Encode an integer as the archive does: a sign byte plus intSize magnitude +# bytes (we set intSize = 1 in the header, so one byte covers these values). +sub toc_int +{ + my ($v) = @_; + return pack('CC', ($v < 0 ? 1 : 0), abs($v) & 0xFF); +} + +# Encode a string, or a NULL field (negative length) when undef. +sub toc_str +{ + my ($s) = @_; + return defined $s ? toc_int(length $s) . $s : toc_int(-1); +} + +# Custom-format header (version 1.12, format 1, intSize/offSize 1) accepted by +# ReadHead(), then a TOC announcing a single entry. +my $toc_header = 'PGDMP' . pack('CCC', 1, 12, 0) . pack('CCC', 1, 1, 1); +$toc_header .= toc_int(0); # compression level (none) +$toc_header .= toc_int(0) x 3; # createDate: sec, min, hour +$toc_header .= toc_int(1) . toc_int(0) . toc_int(100); # mday, mon, year +$toc_header .= toc_int(0); # createDate: isdst +$toc_header .= toc_str(undef) x 3; # dbname, remote and dump version +$toc_header .= toc_int(1); # tocCount + +# Every entry starts with dumpId and hadDumper. +my $toc_entry = toc_int(1) . toc_int(0); + +# Each case supplies valid required strings up to the field under test, which is +# then left NULL. The names match the ReadRequiredStr() call sites in ReadToc(). +my @toc_cases = ( + [ 'table OID', '' ], + [ 'OID', toc_str('0') ], + [ 'entry tag', toc_str('0') . toc_str('0') ], + [ 'entry description', toc_str('0') . toc_str('0') . toc_str('t') ], + [ + 'WITH OIDS marker', + toc_str('0') . toc_str('0') . toc_str('t') . toc_str('d') + . toc_int(0) # section + . (toc_str(undef) x 6) # defn, dropStmt, copyStmt, namespace, + # tablespace, owner (all nullable) + ], +); + +my $toc_case_no = 0; +foreach my $toc_case (@toc_cases) +{ + my ($field, $prefix) = @$toc_case; + my $file = "$tempdir/corrupt_toc_${toc_case_no}.dump"; + open my $fh, '>:raw', $file or die "could not create $file: $!"; + print $fh $toc_header . $toc_entry . $prefix . toc_str(undef); + close $fh; + + command_fails_like( + [ 'pg_restore', '-l', $file ], + qr/\Qpg_restore: error: missing $field in TOC -- perhaps a corrupt TOC\E/, + "pg_restore: NULL $field in TOC reported as corrupt"); + $toc_case_no++; +} + done_testing(); -- 2.53.0