Re: doPickSplit stack buffer overflow in XLogInsert?

On Wed, Nov 27, 2013 at 11:38:23AM -0800, Kevin Grittner wrote:
> Noah Misch <noah(at)leadboat(dot)com> wrote:
> > The threat is that rounding the read size up to the next MAXALIGN
> > would cross into an unreadable memory page, resulting in a
> > SIGSEGV.  Every palloc chunk has MAXALIGN'd size under the hood,
> > so the excess read of "toDelete" cannot cause a SIGSEGV.  For a
> > stack variable, it depends on the ABI.  I'm not aware of an ABI
> > where the four bytes past the end of this stack variable could be
> > unreadable, which is not to claim I'm well-read on the topic.  We
> > should fix this in due course on code hygiene grounds, but I
> > would not back-patch it.
>
> If you're sure.  I hadn't worked through the code, but had two
> concerns (neither of which was about a SEGSEGV):
>
> (1)  That multiple MAXALIGNs of shorter values could push the
> structure into overlap with the next thing on the stack, allowing
> one or the other to get stepped on.

These are out-of-bounds reads only, not writes. Also, the excess doesn't
accumulate that way; the code reads beyond any particular stack variable or
palloc chunk by no more than 7 bytes.

> (2)  That the CRC calculation might picking up uninitialized data
> which was not actually going to match what was used during
> recovery, leading to "end of recovery" on replay.

The CRC calculation does pick up unspecified bytes, but we copy those same
bytes into the actual WAL record. The effect is similar to that of
unspecified pad bytes in the middle of xlrec structures.

> If you are confident that neither of these is a real risk, I'll
> relax about this.

If there is a real risk, I'm not seeing it.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com