Re: BUG #19613: pg_restore: several SEGVs in ReadToc() in pg_backup_archiver.c

Hi Andrey,

> The attached patch adds a small ReadRequiredStr() helper for fields
> that must be present in a valid archive, and keeps ReadStr() for the
> nullable cases.

I've read the patch. The distinction between five required strings and
the remaining nullable fields looks correct to me.

Could we add a regression test? The reporter's reproducer is short and
looks cool, needs no server, and could fit naturally in t/001_basic.pl. A
command_fails_like() check for the new "missing table OID in TOC" error
would prove that the original crash is fixed and protect the new helper.

It might be worth covering the other four call sites too, but I would
consider the original reproducer sufficient for this patch.

I found one earlier report of this exact failure, from 2021 [0]. In that
case, ssh -t put pg_dump through a pseudo-terminal and corrupted the
binary stream; pg_restore then crashed at the same sscanf(NULL) in
ReadToc(). The NULL case came up again in a 2022 discussion [1], but was
left unfixed partly because valid pg_dump output cannot reach it.

So this is an old defect with at least one real user report, not only a
synthetic malformed input. That seems like a good reason to back-patch
the fix to all supported branches. The helper keeps the change small,
and valid archives retain exactly the same behavior.

Apart from the missing test, the fix looks good to me.

Thank you!

Best regards, Andrey Borodin.

[0] https://postgr.es/m/CF8A97DB-240C-4E9E-826D-743D6AD1C27B@legalserver.org
[1] https://postgr.es/m/70019E5D-A6AB-43BA-84F9-D36EB8C678B6@yesql.se