Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On 05/06/2014 07:36 PM, Andres Freund wrote: On 2014-05-06 13:33:01 +0300, Heikki Linnakangas wrote: On 03/31/2014 09:08 PM, Robert Haas wrote: On Wed, Mar 26, 2014 at 9:45 PM, Peter Geoghegan p...@heroku.com wrote: On Wed, Nov 27, 2013 at 9:10 AM, Noah Misch n...@leadboat.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. y Not knowing anything about spgist this looks sane to me. Since we did a catversion bump anyway, I revisited this. Committed an expanded version of the patch I posted earlier. I went through all the SP-GiST WAL record types and removed the alignment requirements of tuples from all of them. Most of them didn't have a problem because the structs happened to have suitable alignment by accident, but seems best to not rely on that, for the sake of consistency and robustness if the structs are modified later. I ran this through my testing tool that compares page image on master and standby after every WAL record replay, with full_page_writes on and off, and found no errors. Do we need a backpatchable solution given that we seem to agree that these bugs aren't likely to cause harm? I don't think we do. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On 03/31/2014 09:08 PM, Robert Haas wrote:
On Wed, Mar 26, 2014 at 9:45 PM, Peter Geoghegan p...@heroku.com wrote:
On Wed, Nov 27, 2013 at 9:10 AM, Noah Misch n...@leadboat.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
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index 48f32cd..ff403ef 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -533,9 +533,9 @@ moveLeafs(Relation index, SpGistState *state,
{
XLogRecPtr recptr;
- ACCEPT_RDATA_DATA(xlrec, MAXALIGN(sizeof(xlrec)), 0);
- ACCEPT_RDATA_DATA(toDelete, MAXALIGN(sizeof(OffsetNumber) * nDelete), 1);
- ACCEPT_RDATA_DATA(toInsert, MAXALIGN(sizeof(OffsetNumber) * nInsert), 2);
+ ACCEPT_RDATA_DATA(xlrec, SizeOfSpgxlogMoveLeafs, 0);
+ ACCEPT_RDATA_DATA(toDelete, sizeof(OffsetNumber) * nDelete, 1);
+ ACCEPT_RDATA_DATA(toInsert, sizeof(OffsetNumber) * nInsert, 2);
ACCEPT_RDATA_DATA(leafdata, leafptr - leafdata, 3);
ACCEPT_RDATA_BUFFER(current-buffer, 4);
ACCEPT_RDATA_BUFFER(nbuf, 5);
@@ -1116,7 +1116,7 @@ doPickSplit(Relation index, SpGistState *state,
leafdata = leafptr = (char *) palloc(totalLeafSizes);
- ACCEPT_RDATA_DATA(xlrec, MAXALIGN(sizeof(xlrec)), 0);
+ ACCEPT_RDATA_DATA(xlrec, SizeOfSpgxlogPickSplit, 0);
ACCEPT_RDATA_DATA(innerTuple, innerTuple-size, 1);
nRdata = 2;
@@ -1152,7 +1152,7 @@ doPickSplit(Relation index, SpGistState *state,
{
xlrec.nDelete = nToDelete;
ACCEPT_RDATA_DATA(toDelete,
- MAXALIGN(sizeof(OffsetNumber) * nToDelete),
+ sizeof(OffsetNumber) * nToDelete,
nRdata);
nRdata++;
ACCEPT_RDATA_BUFFER(current-buffer, nRdata);
@@ -1251,13 +1251,9 @@ doPickSplit(Relation index, SpGistState *state,
}
xlrec.nInsert = nToInsert;
- ACCEPT_RDATA_DATA(toInsert,
- MAXALIGN(sizeof(OffsetNumber) * nToInsert),
- nRdata);
+ ACCEPT_RDATA_DATA(toInsert, sizeof(OffsetNumber) * nToInsert, nRdata);
nRdata++;
- ACCEPT_RDATA_DATA(leafPageSelect,
- MAXALIGN(sizeof(uint8) * nToInsert),
- nRdata);
+ ACCEPT_RDATA_DATA(leafPageSelect, sizeof(uint8) * nToInsert, nRdata);
nRdata++;
ACCEPT_RDATA_DATA(leafdata, leafptr - leafdata, nRdata);
nRdata++;
diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c
index 1689324..cd6a4b2 100644
--- a/src/backend/access/spgist/spgxlog.c
+++ b/src/backend/access/spgist/spgxlog.c
@@ -205,7 +205,7 @@ static void
spgRedoMoveLeafs(XLogRecPtr lsn, XLogRecord *record)
{
char *ptr = XLogRecGetData(record);
- spgxlogMoveLeafs *xldata = (spgxlogMoveLeafs *) ptr;
+ spgxlogMoveLeafs *xldata;
SpGistState state;
OffsetNumber *toDelete;
OffsetNumber *toInsert;
@@ -213,18 +213,19 @@ spgRedoMoveLeafs(XLogRecPtr lsn, XLogRecord *record)
Buffer buffer;
Page page;
- fillFakeState(state, xldata-stateSrc);
-
+ xldata = (spgxlogMoveLeafs *) ptr;
nInsert = xldata-replaceDead ? 1 : xldata-nMoves + 1;
- ptr += MAXALIGN(sizeof(spgxlogMoveLeafs));
+ ptr += SizeOfSpgxlogMoveLeafs;
toDelete = (OffsetNumber *) ptr;
- ptr += MAXALIGN(sizeof(OffsetNumber) * xldata-nMoves);
+ ptr += sizeof(OffsetNumber) * xldata-nMoves;
toInsert = (OffsetNumber *) ptr;
- ptr += MAXALIGN(sizeof(OffsetNumber) * nInsert);
+ ptr += sizeof(OffsetNumber) * nInsert;
/* now ptr points to the list of leaf tuples */
+ fillFakeState(state, xldata-stateSrc);
+
/*
* In normal operation we would have all three pages (source, dest, and
* parent) locked simultaneously; but in WAL replay it should be safe to
@@ -252,10 +253,16 @@ spgRedoMoveLeafs(XLogRecPtr lsn, XLogRecord *record)
for (i = 0; i nInsert; i++)
{
- SpGistLeafTuple lt =
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On 2014-05-06 13:33:01 +0300, Heikki Linnakangas wrote: On 03/31/2014 09:08 PM, Robert Haas wrote: On Wed, Mar 26, 2014 at 9:45 PM, Peter Geoghegan p...@heroku.com wrote: On Wed, Nov 27, 2013 at 9:10 AM, Noah Misch n...@leadboat.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. Not knowing anything about spgist this looks sane to me. Do we need a backpatchable solution given that we seem to agree that these bugs aren't likely to cause harm? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
Hi, We really should fix this one of these days. On 2014-03-26 18:45:54 -0700, Peter Geoghegan wrote: 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. I don't think that's entirely sufficient. The local spgxlogPickSplit xlrec;/spgxlogMoveLeafs xlrec; variables are also inserted while MAXLIGNing their size. That's slightly harder to fix :(. I don't have a better idea than also allocating them dynamically :( Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On Wed, Mar 26, 2014 at 9:45 PM, Peter Geoghegan p...@heroku.com wrote: On Wed, Nov 27, 2013 at 9:10 AM, Noah Misch n...@leadboat.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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On Wed, Nov 27, 2013 at 9:10 AM, Noah Misch n...@leadboat.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. -- Peter Geoghegan *** a/src/backend/access/spgist/spgdoinsert.c --- b/src/backend/access/spgist/spgdoinsert.c *** moveLeafs(Relation index, SpGistState *s *** 412,419 /* Locate the tuples to be moved, and count up the space needed */ i = PageGetMaxOffsetNumber(current-page); ! toDelete = (OffsetNumber *) palloc(sizeof(OffsetNumber) * i); ! toInsert = (OffsetNumber *) palloc(sizeof(OffsetNumber) * (i + 1)); size = newLeafTuple-size + sizeof(ItemIdData); --- 412,419 /* Locate the tuples to be moved, and count up the space needed */ i = PageGetMaxOffsetNumber(current-page); ! toDelete = (OffsetNumber *) palloc0(MAXALIGN(sizeof(OffsetNumber) * i)); ! toInsert = (OffsetNumber *) palloc0(MAXALIGN(sizeof(OffsetNumber) * (i + 1))); size = newLeafTuple-size + sizeof(ItemIdData); *** doPickSplit(Relation index, SpGistState *** 722,731 n = max + 1; in.datums = (Datum *) palloc(sizeof(Datum) * n); heapPtrs = (ItemPointerData *) palloc(sizeof(ItemPointerData) * n); ! toDelete = (OffsetNumber *) palloc(sizeof(OffsetNumber) * n); ! toInsert = (OffsetNumber *) palloc(sizeof(OffsetNumber) * n); newLeafs = (SpGistLeafTuple *) palloc(sizeof(SpGistLeafTuple) * n); ! leafPageSelect = (uint8 *) palloc(sizeof(uint8) * n); xlrec.node = index-rd_node; STORE_STATE(state, xlrec.stateSrc); --- 722,731 n = max + 1; in.datums = (Datum *) palloc(sizeof(Datum) * n); heapPtrs = (ItemPointerData *) palloc(sizeof(ItemPointerData) * n); ! toDelete = (OffsetNumber *) palloc0(MAXALIGN(sizeof(OffsetNumber) * n)); ! toInsert = (OffsetNumber *) palloc0(MAXALIGN(sizeof(OffsetNumber) * n)); newLeafs = (SpGistLeafTuple *) palloc(sizeof(SpGistLeafTuple) * n); ! leafPageSelect = (uint8 *) palloc0(MAXALIGN(sizeof(uint8) * n)); xlrec.node = index-rd_node; STORE_STATE(state, xlrec.stateSrc); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
Andres Freund and...@2ndquadrant.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 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? -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On Wed, Nov 27, 2013 at 06:23:38AM -0800, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
Noah Misch n...@leadboat.com wrote: (Kevin, I saw no attachment.) Apologies. Trying again. 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. (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. If you are confident that neither of these is a real risk, I'll relax about this. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company= ==7232==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffbcb2e834 at pc 0xef0335 bp 0x7fffbcb29b30 sp 0x7fffbcb29b28 READ of size 1 at 0x7fffbcb2e834 thread T0 #0 0xef0334 in XLogInsert /home/kgrittn/pg/master/src/backend/access/transam/xlog.c:1040 #1 0xd53792 in doPickSplit /home/kgrittn/pg/master/src/backend/access/spgist/spgdoinsert.c:1391 #2 0xd0d9f1 in spgdoinsert /home/kgrittn/pg/master/src/backend/access/spgist/spgdoinsert.c:2008 #3 0xcbb4d1 in spginsert /home/kgrittn/pg/master/src/backend/access/spgist/spginsert.c:238 #4 0x46f9de3 in FunctionCall6Coll /home/kgrittn/pg/master/src/backend/utils/fmgr/fmgr.c:1436 #5 0xad43fb in index_insert /home/kgrittn/pg/master/src/backend/access/index/indexam.c:223 #6 0x2122fcd in ExecInsertIndexTuples /home/kgrittn/pg/master/src/backend/executor/execUtils.c:1104 #7 0x228413f in ExecInsert /home/kgrittn/pg/master/src/backend/executor/nodeModifyTable.c:274 #8 0x227fba8 in ExecModifyTable /home/kgrittn/pg/master/src/backend/executor/nodeModifyTable.c:1014 #9 0x2026b03 in ExecProcNode /home/kgrittn/pg/master/src/backend/executor/execProcnode.c:377 #10 0x1fef534 in ExecutePlan /home/kgrittn/pg/master/src/backend/executor/execMain.c:1474 #11 0x1fee488 in standard_ExecutorRun /home/kgrittn/pg/master/src/backend/executor/execMain.c:308 #12 0x1fec7e9 in ExecutorRun /home/kgrittn/pg/master/src/backend/executor/execMain.c:256 #13 0x34f05ab in ProcessQuery /home/kgrittn/pg/master/src/backend/tcop/pquery.c:185 #14 0x34e8c3a in PortalRunMulti /home/kgrittn/pg/master/src/backend/tcop/pquery.c:1279 #15 0x34e1b9c in PortalRun /home/kgrittn/pg/master/src/backend/tcop/pquery.c:816 #16 0x34b6721 in exec_simple_query /home/kgrittn/pg/master/src/backend/tcop/postgres.c:1054 #17 0x34b1420 in PostgresMain /home/kgrittn/pg/master/src/backend/tcop/postgres.c:3998 #18 0x2f1f925 in BackendRun /home/kgrittn/pg/master/src/backend/postmaster/postmaster.c:4085 #19 0x2f1b830 in BackendStartup /home/kgrittn/pg/master/src/backend/postmaster/postmaster.c:3774 #20 0x2efcc96 in ServerLoop /home/kgrittn/pg/master/src/backend/postmaster/postmaster.c:1585 #21 0x2ef13ee in PostmasterMain /home/kgrittn/pg/master/src/backend/postmaster/postmaster.c:1240 #22 0x24dc3c3 in main /home/kgrittn/pg/master/src/backend/main/main.c:196 #23 0x2b89d526e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #24 0x4dc3cc in _start ??:? Address 0x7fffbcb2e834 is located in stack of thread T0 at offset 2644 in frame #0 0xd2b5ef in doPickSplit /home/kgrittn/pg/master/src/backend/access/spgist/spgdoinsert.c:680 This frame has 57 object(s): [32, 40) '' [96, 104) '' [160, 168) '' [224, 232) '' [288, 296) '' [352, 356) '' [416, 417) '' [480, 481) '' [544, 545) 'insertedNew' [608, 632) 'in' [672, 720) 'out' [768, 776) 'procinfo' [832, 833) 'includeNew' [896, 900) 'i' [960, 964) 'max' [1024, 1028) 'n' [1088, 1096) 'innerTuple' [1152, 1160) 'node' [1216, 1224) 'nodes' [1280, 1284) 'newInnerBuffer' [1344, 1348) 'newLeafBuffer' [1408, 1416) 'heapPtrs' [1472, 1480) 'leafPageSelect' [1536, 1544) 'leafSizes' [1600, 1608) 'toDelete' [1664, 1672) 'toInsert' [1728, 1730) 'redirectTuplePos' [1792, 1796) 'startOffsets' [1856, 1864) 'newLeafs' [1920, 1924) 'spaceToDelete' [1984, 1988) 'currentFreeSpace' [2048, 2052) 'totalLeafSizes' [2112, 2113) 'allTheSame' [2176, 2496) 'rdata' [2528, 2532) 'nRdata' [2592, 2644) 'xlrec' == Memory access
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On Wed, Nov 27, 2013 at 11:38:23AM -0800, Kevin Grittner wrote: Noah Misch n...@leadboat.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On 2013-11-27 15:29:24 -0500, Noah Misch wrote: 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. Me neither. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
On 11/26/13, 5:14 PM, 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 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. I also see that. It only happens in 64-bit builds. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] doPickSplit stack buffer overflow in XLogInsert?
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 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. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doPickSplit stack buffer overflow in XLogInsert?
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 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. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
