Horton

Re: BUG #17928: Standby fails to decode WAL on termination of primary

Thomas Munro <thomas.munro@gmail.com>

View thread
From: Thomas Munro <thomas.munro@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, Alexander Lakhin <exclusion@gmail.com>, Sergei Kornilov <sk@zsrv.org>, Noah Misch <noah@leadboat.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-bugs@lists.postgresql.org, Fujii Masao <masao.fujii@oss.nttdata.com>, pgbf@twiska.com
Date: 2023-09-25T05:05:03Z
Lists: pgsql-bugs

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Correct assertion and comments about XLogRecordMaxSize.

  2. Fix edge-case for xl_tot_len broken by bae868ca.

  3. Don't use Perl pack('Q') in 039_end_of_wal.pl.

  4. Don't trust unvalidated xl_tot_len.

  5. Make recovery report error message when invalid page header is found.

  6. Add more protections in WAL record APIs against overflows

Attachments

On Mon, Sep 25, 2023 at 5:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Sure looks like ValidXLogRecord is assuming that record->xl_tot_len
> can be trusted without reservation.

I think there is something at least theoretically a bit fishy about
the ancient code that re-reads the page, doesn't consider short reads,
and assumes that record->xl_tot_len hasn't changed since last time it
loaded and validated it.  But I think that's also very unlikely to go
wrong, and it's not the problem here.

The problem is that bae868ca removed something we still need:

-               /* XXX: more validation should be done here */
-               if (total_len < SizeOfXLogRecord)
-               {
-                       report_invalid_record(state,
-
"invalid record length at %X/%X: expected at least %u, got %u",
-
LSN_FORMAT_ARGS(RecPtr),
-
(uint32) SizeOfXLogRecord, total_len);
-                       goto err;
-               }
+               /* We'll validate the header once we have the next page. */
                gotheader = false;

If you happened to run into zeroes where an xl_tot_len is wanted right
at the end of a page (or any value not big enough to get you to the
next page), we'll fall through to the single-page branch, and then go
directly to the CRC check, but then ValidXLogRecord() subtracts
SizeOfXLogRecord and gets a crazy big length.  The CRC implementation
routines on modern computers happened to use pointer arithmetic that
terminates immediately without accessing any memory, which is why
nothing was obviously wrong on most systems.  The _sb8.c
implementation for older ARM, MIPS etc use a length-based loop, and
read off into deep space.

Draft patch attached, including a new test for 039_end_of_wal.pl that
fails on all systems without the above code.