Re: doPickSplit stack buffer overflow in XLogInsert?

On Wed, Nov 27, 2013 at 06:23:38AM -0800, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-11-26 14:14:38 -0800, Kevin Grittner wrote:
> >
> >> I happened to build in a shell that was still set up for the clang
> >> address sanitizer, and got the attached report.  On a rerun it was

(Kevin, I saw no attachment.)

> >> repeatable.  XLogInsert() seems to read past the end of a variable
> >> allocated on the stack in doPickSplit(). I haven't tried to analyze
> >> it past that, since this part of the code is unfamiliar to me.
> >
> > Yea, I've seen that one before as well and planned to report it at some
> > point. The reason is the MAXALIGN()s in ACCEPT_RDATA_DATA(). That rounds
> > up to 8byte boundaries, while we've e.g. only added 2bytes of slop to
> > toDelete.
>
> Have you established whether having the CRC calculation read past
> the end of the buffer can cause problems on recovery or standby
> systems?  Should we try to get this fixed by Monday?

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.

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