Thread: [PATCH] Fix crash in int8_avg_combine().
While doing some tests on REL_10_STABLE, I was getting run-time exceptions at int8_avg_combine() at the following line:
state1->sumX = state2->sumX;
After some debugging, I noticed that palloc()’s alignment is 8-bytes, while this statement (which moves a __int128 from one memory location to another memory location) expects 16-byte memory alignments. So when either state1 or state2 is not 16-byte aligned, this crashes.
When I disassemble the code, the above statement is translated to a pair of movdqa and movaps assignments when compiled with -O2:
movdqa c(%rdx), %xmm0
movaps %xmm0, c(%rcx)
Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 2”, both of these instructions expect 16-byte aligned memory locations, or a general-protection exception will be generated.
I wasn’t getting the exception always. For example, the same query that crashed in REL_10_STABLE didn’t crash in master. But I think that was just lucky allocation, and we will eventually see cases of this crash in all versions that use __int128 assignment in int8_avg_combine().
I’ve attached a patch which sets the MAXIMUM_ALIGNOF correctly when __int128’s are available, so fixes the crash. This patch is on top of REL_10_STABLE, which is the branch I was getting the crash at. I can create patches for other branches if we think this is the proper change.
The sequence for which I got the crash in REL_10_STABLE was the following sequence of commands after a fresh initdb:
CREATE TABLE t(a BIGINT);
INSERT INTO t SELECT * FROM generate_series(1, 10000000);
SELECT sum(a) FROM t;
I’m not sure if this will crash for everyone, since you can be lucky and have both states assigned 16-byte aligned locations. This was the case for me in master branch. "SELECT version()" is "PostgreSQL 10.0 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2), 64-bit", if that is related.
I notice that there’s a comment in configure.in that says the penalty of using 16-bit alignment might be too much for disk and memory space. If this is the case, then we need to modify numeric.c to make sure that it never use any instructions with 16-byte alignment requirements. I can do that if that is the consensus.
state1->sumX = state2->sumX;
After some debugging, I noticed that palloc()’s alignment is 8-bytes, while this statement (which moves a __int128 from one memory location to another memory location) expects 16-byte memory alignments. So when either state1 or state2 is not 16-byte aligned, this crashes.
When I disassemble the code, the above statement is translated to a pair of movdqa and movaps assignments when compiled with -O2:
movdqa c(%rdx), %xmm0
movaps %xmm0, c(%rcx)
Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 2”, both of these instructions expect 16-byte aligned memory locations, or a general-protection exception will be generated.
I wasn’t getting the exception always. For example, the same query that crashed in REL_10_STABLE didn’t crash in master. But I think that was just lucky allocation, and we will eventually see cases of this crash in all versions that use __int128 assignment in int8_avg_combine().
I’ve attached a patch which sets the MAXIMUM_ALIGNOF correctly when __int128’s are available, so fixes the crash. This patch is on top of REL_10_STABLE, which is the branch I was getting the crash at. I can create patches for other branches if we think this is the proper change.
The sequence for which I got the crash in REL_10_STABLE was the following sequence of commands after a fresh initdb:
CREATE TABLE t(a BIGINT);
INSERT INTO t SELECT * FROM generate_series(1, 10000000);
SELECT sum(a) FROM t;
I’m not sure if this will crash for everyone, since you can be lucky and have both states assigned 16-byte aligned locations. This was the case for me in master branch. "SELECT version()" is "PostgreSQL 10.0 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2), 64-bit", if that is related.
I notice that there’s a comment in configure.in that says the penalty of using 16-bit alignment might be too much for disk and memory space. If this is the case, then we need to modify numeric.c to make sure that it never use any instructions with 16-byte alignment requirements. I can do that if that is the consensus.
Or maybe we can create an allocation function for 16-byte aligned allocations.
Any thoughts?
Attachment
Hi Hadi, On 2017-11-25 22:43:49 -0500, Hadi Moshayedi wrote: > While doing some tests on REL_10_STABLE, I was getting run-time exceptions > at int8_avg_combine() at the following line: > > state1->sumX = state2->sumX; > > After some debugging, I noticed that palloc()’s alignment is 8-bytes, while > this statement (which moves a __int128 from one memory location to another > memory location) expects 16-byte memory alignments. So when either state1 > or state2 is not 16-byte aligned, this crashes. > > When I disassemble the code, the above statement is translated to a pair of > movdqa and movaps assignments when compiled with -O2: > > movdqa c(%rdx), %xmm0 > movaps %xmm0, c(%rcx) > > Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual, > Volume 2”, both of these instructions expect 16-byte aligned memory > locations, or a general-protection exception will be generated. Nicely analyzed. [Un]fortunately the bug has already been found and fixed: https://git.postgresql.org/pg/commitdiff/619a8c47da7279c186bb57cc16b26ad011366b73 Will be included in the next set of back branch releases. > diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h > index 869c59dc85..2dc59e89cd 100644 > --- a/src/include/utils/memutils.h > +++ b/src/include/utils/memutils.h > @@ -189,7 +189,7 @@ extern MemoryContext SlabContextCreate(MemoryContext parent, > * Few callers should be interested in this, but tuplesort/tuplestore need > * to know it. > */ > -#define ALLOCSET_SEPARATE_THRESHOLD 8192 > +#define ALLOCSET_SEPARATE_THRESHOLD 16384 Huh, what's that about in this context? Greetings, Andres Freund
Hadi Moshayedi <hadi@moshayedi.net> writes: > While doing some tests on REL_10_STABLE, I was getting run-time exceptions > at int8_avg_combine() at the following line: > state1->sumX = state2->sumX; > After some debugging, I noticed that palloc()’s alignment is 8-bytes, while > this statement (which moves a __int128 from one memory location to another > memory location) expects 16-byte memory alignments. So when either state1 > or state2 is not 16-byte aligned, this crashes. I believe we already dealt with this: Author: Tom Lane <tgl@sss.pgh.pa.us> Branch: REL_10_STABLE [619a8c47d] 2017-11-14 17:49:49 -0500 Branch: REL9_6_STABLE [4a15f87d2] 2017-11-14 17:49:49 -0500 Branch: REL9_5_STABLE [d4e38489f] 2017-11-14 17:49:49 -0500 Prevent int128 from requiring more than MAXALIGN alignment. Our initial work with int128 neglected alignment considerations,an oversight that came back to bite us in bug #14897 from Vincent Lachenal. It is unsurprising that int128might have a 16-byte alignment requirement; what's slightly more surprising is that even notoriously lax Intel chips sometimes enforce that. Raising MAXALIGN seems out of the question: the costs in wasted disk and memory spacewould be significant, and there would also be an on-disk compatibility break. Nor does it seem very practical totry to allow some data structures to have more-than-MAXALIGN alignment requirement, as we'd have to push knowledgeof that throughout various code that copies data structures around. The only way out of the box is to maketype int128 conform to the system's alignment assumptions. Fortunately, gcc supports that via its __attribute__(aligned())pragma; and since we don't currently support int128 on non-gcc-workalike compilers, we shouldn'tbe losing any platform support this way. Although we could have just done pg_attribute_aligned(MAXIMUM_ALIGNOF)and called it a day, I did a little bit of extra work to make the code more portablethan that: it will also support int128 on compilers without __attribute__(aligned()), if the native alignment oftheir 128-bit-int type is no more than that of int64. Add a regression test case that exercises the one known instanceof the problem, in parallel aggregation over a bigint column. Back-patch of commit 751804998. The code knownto be affected only exists in 9.6 and later, but we do have some stuff using int128 in 9.5, so patch back to 9.5. Discussion: https://postgr.es/m/20171110185747.31519.28038@wrigleys.postgresql.org Does that solution not work for you? regards, tom lane
On Sat, Nov 25, 2017 at 10:55 PM, Tom Lane wrote:
> I believe we already dealt with this:
>
> Author: Tom Lane
> Branch: REL_10_STABLE [619a8c47d] 2017-11-14 17:49:49 -0500
> Branch: REL9_6_STABLE [4a15f87d2] 2017-11-14 17:49:49 -0500
> Branch: REL9_5_STABLE [d4e38489f] 2017-11-14 17:49:49 -0500
>
> Prevent int128 from requiring more than MAXALIGN alignment.
>
> Our initial work with int128 neglected alignment considerations, an
> oversight that came back to bite us in bug #14897 from Vincent
> Lachenal.
> It is unsurprising that int128 might have a 16-byte alignment
> requirement;
> what's slightly more surprising is that even notoriously lax Intel
> chips
> sometimes enforce that.
>
> Raising MAXALIGN seems out of the question: the costs in wasted disk
> and
> memory space would be significant, and there would also be an on-disk
> compatibility break. Nor does it seem very practical to try to allow
> some
> data structures to have more-than-MAXALIGN alignment requirement, as
> we'd
> have to push knowledge of that throughout various code that copies data
> structures around.
>
> The only way out of the box is to make type int128 conform to the
> system's
> alignment assumptions. Fortunately, gcc supports that via its
> __attribute__(aligned()) pragma; and since we don't currently support
> int128 on non-gcc-workalike compilers, we shouldn't be losing any
> platform
> support this way.
>
> Although we could have just done pg_attribute_aligned(MAXIMUM_ALIGNOF)
> and
> called it a day, I did a little bit of extra work to make the code more
> portable than that: it will also support int128 on compilers without
> __attribute__(aligned()), if the native alignment of their 128-bit-int
> type is no more than that of int64.
>
> Add a regression test case that exercises the one known instance of the
> problem, in parallel aggregation over a bigint column.
>
> Back-patch of commit 751804998. The code known to be affected only
> exists
> in 9.6 and later, but we do have some stuff using int128 in 9.5, so
> patch
> back to 9.5.
>
> Discussion: https://postgr.es/m/20171110185747.31519.28038@
> wrigleys.postgresql.org
>
> Does that solution not work for you?
>
It works for me. My REL_10_STABLE was out of date. A git pull fixed it.
On Sat, Nov 25, 2017 at 10:47 PM, Andres Freund
wrote:
> diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
> > index 869c59dc85..2dc59e89cd 100644
> > --- a/src/include/utils/memutils.h
> > +++ b/src/include/utils/memutils.h
> > @@ -189,7 +189,7 @@ extern MemoryContext SlabContextCreate(MemoryContext
> parent,
> > * Few callers should be interested in this, but tuplesort/tuplestore
> need
> > * to know it.
> > */
> > -#define ALLOCSET_SEPARATE_THRESHOLD 8192
> > +#define ALLOCSET_SEPARATE_THRESHOLD 16384
>
> Huh, what's that about in this context?
>
There is following static assert:
StaticAssertStmt(ALLOC_CHUNK_LIMIT == ALLOCSET_SEPARATE_THRESHOLD, ...)
and ALLOCK_CHUNK_LIMIT is defined as:
#define ALLOC_CHUNK_LIMIT (1 << (ALLOCSET_NUM_FREELISTS-1+ALLOC_MINBITS))
and there is this comment:
"CAUTION: ALLOC_MINBITS must be large enough so that 1<