Обсуждение: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

[ redirecting to -hackers ]

Alexander Korotkov <aekorotkov@gmail.com> writes:
>> BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
>> -fsanitize=alignment -fsanitize-trap=alignment" and the patch
>> attached.
>> I can propose the following to catch such issues earlier.  We could
>> finish (wrap attribute with macro and apply it to other places with
>> misalignment access if any) and apply the attached patch and make
>> commitfest.cputube.org check patches with CFLAGS="-O0
>> -fsanitize=alignment -fsanitize-trap=alignment".  What do you think?

> The revised patch is attached.  The attribute is wrapped into
> pg_attribute_no_sanitize_alignment() macro.  I've checked it works for
> me with gcc-10 and clang-11.

I found some time to experiment with this today.  It is really nice
to be able to detect these problems without using obsolete hardware.
However, I have a few issues:

* Why do you recommend -O0?  Seems to me we want to test the code
as we'd normally use it, ie typically -O2.

* -fsanitize-trap=alignment seems to be a clang-ism; gcc won't take it.
However, after some experimenting I found that "-fno-sanitize-recover=all"
(or "-fno-sanitize-recover=alignment" if you prefer) produces roughly
equivalent results on gcc.

* Both clang and gcc seem to be happy with the same spelling of the
function attribute, which is fortunate.  However, I seriously doubt
that bare "#ifdef __GNUC__" is going to be good enough.  At the very
least there's going to need to be a compiler version test in there,
and we might end up needing to get the configure script involved.

* I think the right place to run such a check is in some buildfarm
animals.  The cfbot only sees portions of what goes into our tree.

            regards, tom lane

I wrote:
> * Both clang and gcc seem to be happy with the same spelling of the
> function attribute, which is fortunate.  However, I seriously doubt
> that bare "#ifdef __GNUC__" is going to be good enough.  At the very
> least there's going to need to be a compiler version test in there,
> and we might end up needing to get the configure script involved.

After digging in gcc's release history, it seems they invented
"-fsanitize=alignment" in GCC 5, so we can make this work for gcc
by writing

#if __GNUC__ >= 5

(the likely() macro already uses a similar approach).  Can't say
if that's close enough for clang too.

            regards, tom lane

I wrote:
> After digging in gcc's release history, it seems they invented
> "-fsanitize=alignment" in GCC 5, so we can make this work for gcc
> by writing
> #if __GNUC__ >= 5
> (the likely() macro already uses a similar approach).  Can't say
> if that's close enough for clang too.

Ugh, no it isn't: even pretty recent clang releases only define
__GNUC__ as 4.  It looks like we need a separate test on clang's
version.  I looked at their version history and sanitizers seem
to have come in around clang 7, so I propose the attached (where
I worked a bit harder on the comment, too).

            regards, tom lane

diff --git a/src/include/c.h b/src/include/c.h
index ae978830da..a86342093e 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -132,6 +132,18 @@
 #define pg_nodiscard
 #endif

+/*
+ * Place this macro before functions that should be allowed to make misaligned
+ * accesses.  Think twice before using it on non-x86-specific code!
+ * Testing can be done with "-fsanitize=alignment -fsanitize-trap=alignment"
+ * on clang, or "-fsanitize=alignment -fno-sanitize-recover=alignment" on gcc.
+ */
+#if __clang_major__ >= 7 || __GNUC__ >= 5
+#define pg_attribute_no_sanitize_alignment() __attribute__((no_sanitize("alignment")))
+#else
+#define pg_attribute_no_sanitize_alignment()
+#endif
+
 /*
  * Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only
  * used in assert-enabled builds, to avoid compiler warnings about unused
diff --git a/src/port/pg_crc32c_sse42.c b/src/port/pg_crc32c_sse42.c
index 3b94a7388a..10fc01e1f0 100644
--- a/src/port/pg_crc32c_sse42.c
+++ b/src/port/pg_crc32c_sse42.c
@@ -18,6 +18,7 @@

 #include "port/pg_crc32c.h"

+pg_attribute_no_sanitize_alignment()
 pg_crc32c
 pg_comp_crc32c_sse42(pg_crc32c crc, const void *data, size_t len)
 {

Hi, Tom!

Thank you for taking care of this.

On Mon, Feb 8, 2021 at 3:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> [ redirecting to -hackers ]
>
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> >> BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
> >> -fsanitize=alignment -fsanitize-trap=alignment" and the patch
> >> attached.
> >> I can propose the following to catch such issues earlier.  We could
> >> finish (wrap attribute with macro and apply it to other places with
> >> misalignment access if any) and apply the attached patch and make
> >> commitfest.cputube.org check patches with CFLAGS="-O0
> >> -fsanitize=alignment -fsanitize-trap=alignment".  What do you think?
>
> > The revised patch is attached.  The attribute is wrapped into
> > pg_attribute_no_sanitize_alignment() macro.  I've checked it works for
> > me with gcc-10 and clang-11.
>
> I found some time to experiment with this today.  It is really nice
> to be able to detect these problems without using obsolete hardware.
> However, I have a few issues:
>
> * Why do you recommend -O0?  Seems to me we want to test the code
> as we'd normally use it, ie typically -O2.

My idea was that with -O0 we can see some unaligned accesses, which
would be optimized away with -O2.  I mean with -O2 we might completely
skip accessing some pointer, which would be accessed in -O0.  However,
this situation is probably very rare.

> * I think the right place to run such a check is in some buildfarm
> animals.  The cfbot only sees portions of what goes into our tree.

Could we have both cfbot + buildfarm animals?

------
Regards,
Alexander Korotkov

On Mon, Feb 8, 2021 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > After digging in gcc's release history, it seems they invented
> > "-fsanitize=alignment" in GCC 5, so we can make this work for gcc
> > by writing
> > #if __GNUC__ >= 5
> > (the likely() macro already uses a similar approach).  Can't say
> > if that's close enough for clang too.
>
> Ugh, no it isn't: even pretty recent clang releases only define
> __GNUC__ as 4.  It looks like we need a separate test on clang's
> version.  I looked at their version history and sanitizers seem
> to have come in around clang 7, so I propose the attached (where
> I worked a bit harder on the comment, too).

Looks good to me.  Thank you for revising!

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Mon, Feb 8, 2021 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ugh, no it isn't: even pretty recent clang releases only define
>> __GNUC__ as 4.  It looks like we need a separate test on clang's
>> version.  I looked at their version history and sanitizers seem
>> to have come in around clang 7, so I propose the attached (where
>> I worked a bit harder on the comment, too).

> Looks good to me.  Thank you for revising!

Were you going to push this, or did you expect me to?

            regards, tom lane

On Tue, Feb 9, 2021 at 1:34 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> Could we have both cfbot + buildfarm animals?

Hi Alexander,

For cfbot, yeah it does seem like a good idea to throw whatever code
sanitiser stuff we can into the automated tests, especially stuff that
isn't prone to false alarms.  Can you please recommend an exact change
to apply to:

https://github.com/macdice/cfbot/blob/master/cirrus/.cirrus.yml

Note that FreeBSD and macOS are using clang (though you might think
the latter is using gcc from its configure output...), and Linux is
using gcc.

On Thu, Feb 11, 2021 at 9:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Mon, Feb 8, 2021 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Ugh, no it isn't: even pretty recent clang releases only define
> >> __GNUC__ as 4.  It looks like we need a separate test on clang's
> >> version.  I looked at their version history and sanitizers seem
> >> to have come in around clang 7, so I propose the attached (where
> >> I worked a bit harder on the comment, too).
>
> > Looks good to me.  Thank you for revising!
>
> Were you going to push this, or did you expect me to?

Thank you for noticing.  I'll commit this today.

------
Regards,
Alexander Korotkov

Hi, Thomas!

On Fri, Feb 12, 2021 at 12:04 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Tue, Feb 9, 2021 at 1:34 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > Could we have both cfbot + buildfarm animals?
> For cfbot, yeah it does seem like a good idea to throw whatever code
> sanitiser stuff we can into the automated tests, especially stuff that
> isn't prone to false alarms.  Can you please recommend an exact change
> to apply to:
>
> https://github.com/macdice/cfbot/blob/master/cirrus/.cirrus.yml
>
> Note that FreeBSD and macOS are using clang (though you might think
> the latter is using gcc from its configure output...), and Linux is
> using gcc.

Thank you for the feedback!
I'll propose a pull-request at github.

------
Regards,
Alexander Korotkov

I've updated buildfarm member longfin to use "-fsanitize=alignment
-fsanitize-trap=alignment", and it just got through a run successfully
with that.  It'd be good perhaps if some other buildfarm owners
followed suit (mumble JIT coverage mumble).

Looking around at other recent reports, it looks like we'll need to tweak
the compiler version cutoffs a bit.  I see for instance that spurfowl,
with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609, is whining:

pg_crc32c_sse42.c:24:1: warning: \342\200\230no_sanitize\342\200\231 attribute directive ignored [-Wattributes]

So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5.  I think
we can wait a little bit for more reports before messing with that,
though.

Once this does settle, should we consider back-patching so that it's
possible to run alignment checks in the back branches too?

            regards, tom lane

On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've updated buildfarm member longfin to use "-fsanitize=alignment
> -fsanitize-trap=alignment", and it just got through a run successfully
> with that.  It'd be good perhaps if some other buildfarm owners
> followed suit (mumble JIT coverage mumble).
>
> Looking around at other recent reports, it looks like we'll need to tweak
> the compiler version cutoffs a bit.  I see for instance that spurfowl,
> with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609, is whining:
>
> pg_crc32c_sse42.c:24:1: warning: \342\200\230no_sanitize\342\200\231 attribute directive ignored [-Wattributes]
>
> So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5.  I think
> we can wait a little bit for more reports before messing with that,
> though.

I've rechecked this in the documentation. no_sanitize attribute seems
to appear since gcc 8.0.  Much later than alignment sanitizer itself.
https://gcc.gnu.org/gcc-8/changes.html
"A new attribute no_sanitize can be applied to functions to instruct
the compiler not to do sanitization of the options provided as
arguments to the attribute. Acceptable values for no_sanitize match
those acceptable by the -fsanitize command-line option."

Yes, let's wait for more feedback from buildfarm and fix the version
requirement.

> Once this does settle, should we consider back-patching so that it's
> possible to run alignment checks in the back branches too?

+1

------
Regards,
Alexander Korotkov

I wrote:
> Looking around at other recent reports, it looks like we'll need to tweak
> the compiler version cutoffs a bit.  I see for instance that spurfowl,
> with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609, is whining:
> ...
> So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5.  I think
> we can wait a little bit for more reports before messing with that,
> though.

Further reports show that gcc 6.x and 7.x also produce warnings,
so I moved the cutoff up to 8.  Hopefully that's good enough.
We could write a configure test instead, but I'd just as soon not
expend configure cycles on this.

            regards, tom lane

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5.  I think
>> we can wait a little bit for more reports before messing with that,
>> though.

> I've rechecked this in the documentation. no_sanitize attribute seems
> to appear since gcc 8.0.  Much later than alignment sanitizer itself.

Yeah, I'd just come to that conclusion from scraping the buildfarm
logs.  Good to see it confirmed in the manual though.

>> Once this does settle, should we consider back-patching so that it's
>> possible to run alignment checks in the back branches too?

> +1

Let's make sure we have a clean set of builds and then do that.

            regards, tom lane

I wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
>> On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Once this does settle, should we consider back-patching so that it's
>>> possible to run alignment checks in the back branches too?

>> +1

> Let's make sure we have a clean set of builds and then do that.

The buildfarm seems to be happy --- the active members that haven't
reported in should be unaffected by this patch, either because their
compiler versions are too old or because they're not x86 architecture.
So I went ahead and back-patched, and have adjusted longfin to apply
the -fsanitize switch in all branches.

(I've checked that 9.6 passes check-world this way, but not the
intermediate branches, so it's possible something will fail...)

            regards, tom lane

On Sun, Feb 14, 2021 at 1:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > Alexander Korotkov <aekorotkov@gmail.com> writes:
> >> On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> Once this does settle, should we consider back-patching so that it's
> >>> possible to run alignment checks in the back branches too?
>
> >> +1
>
> > Let's make sure we have a clean set of builds and then do that.
>
> The buildfarm seems to be happy --- the active members that haven't
> reported in should be unaffected by this patch, either because their
> compiler versions are too old or because they're not x86 architecture.
> So I went ahead and back-patched, and have adjusted longfin to apply
> the -fsanitize switch in all branches.

Perfect, thank you very much!

------
Regards,
Alexander Korotkov