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: Noah Misch <noah@leadboat.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Date: 2023-08-13T03:12:34Z
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

On Sun, Aug 13, 2023 at 9:13 AM Noah Misch <noah@leadboat.com> wrote:
> Any user could call pg_logical_emit_message() to silently terminate the WAL
> stream, which is far worse than the original bug.  So far, I'm seeing one way
> to clearly fix $SUBJECT without that harm.  When a record header spans a page
> boundary, read the next page to reassemble the header.  If
> !ValidXLogRecordHeader() (invalid xl_rmid or xl_prev), treat as end of WAL.
> Otherwise, read the whole record in chunks, calculating CRC.  If CRC is
> invalid, treat as end of WAL.  Otherwise, ereport(FATAL) for the oversized
> record.  A side benefit would be avoiding useless large allocations (1GB
> backend, 4GB frontend) as discussed upthread.  May as well do the xl_rmid and
> xl_prev checks in all branches, to avoid needless XLogRecordMaxSize-1
> allocations.  Thoughts?  For invalid-length records in v16+, since every such
> record is end-of-wal or corruption, those versions could skip the CRC.

That sounds quite strong.  But... why couldn't the existing
xlp_rem_len cross-check protect us from this failure mode?  If we
could defer the allocation until after that check (and the usual
ValidXLogRecordHeader() check), I think we'd know that we're really
looking at a size that was actually written in both pages (which must
also have survived xlp_pageaddr check), no?