From: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com>, Peter Geoghegan <pg(at)heroku(dot)com> |
Cc: | Noah Misch <noah(at)leadboat(dot)com>, Kevin Grittner <kgrittn(at)ymail(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: doPickSplit stack buffer overflow in XLogInsert? |
Date: | 2014-05-06 10:33:01 |
Message-ID: | 5368BA5D.2030806@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 03/31/2014 09:08 PM, Robert Haas wrote:
> On Wed, Mar 26, 2014 at 9:45 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> On Wed, Nov 27, 2013 at 9:10 AM, 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.
>>
>> Attached patch silences the "Invalid read of size n" complaints of
>> Valgrind. I agree with your general thoughts around backpatching. Note
>> that the patch addresses a distinct complaint from Kevin's, as
>> Valgrind doesn't take issue with the invalid reads past the end of
>> spgxlogPickSplit variables on the stack.
>
> Is the needless zeroing this patch introduces apt to cause a
> performance problem?
>
> This function is actually pretty wacky. If we're stuffing bytes with
> undefined contents into the WAL record, maybe the answer isn't to
> force the contents of those bytes to be defined, but rather to elide
> them from the WAL record.
Agreed. I propose the attached, which removes the MAXALIGNs. It's not
suitable for backpatching, though, as it changes the format of the WAL
record.
- Heikki
Attachment | Content-Type | Size |
---|---|---|
spgist-wal-remove-maxaligns-1.patch | text/x-diff | 7.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2014-05-06 10:46:55 | Re: Sequential disk access during VACUUM for GiST/GIN |
Previous Message | Dave Page | 2014-05-06 08:36:17 | Re: Possible fix for occasional failures on castoroides etc |