pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: marxin.liska@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15121: Multiple UBSAN errors
Date: Sat, 29 Jun 2019 14:03:34 -0700
Message-ID: <20190629210334.GA1244217@rfd.leadboat.com> (raw )
In-Reply-To: <20078.1521421379@sss.pgh.pa.us >
References: <152140318583.11955.7731957058248462341@wrigleys.postgresql.org >
<7df78014-c931-6c75-6f92-ec48f390b2b9@2ndquadrant.com >
<20078.1521421379@sss.pgh.pa.us >
On Sun, Mar 18, 2018 at 09:02:59PM -0400, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> > On 03/18/2018 08:59 PM, PG Bug reporting form wrote:
> >> arrayfuncs.c:3740:17: runtime error: member access within misaligned address
> >> 0x0000028b937c for type 'struct ExpandedObjectHeader', which requires 8 byte
> >> alignment
> What I think might be happening is that the compiler is taking the
> fact that the pointer is declared as AnyArrayType *, where
>
> typedef union AnyArrayType
> {
> ArrayType flt;
> ExpandedArrayHeader xpn;
> } AnyArrayType;
>
> to assume that the pointer must be aligned on an 8-byte boundary because
> ExpandedArrayHeader would require that, even if we're only accessing the
> "flt" member.
Yep.
> Maybe that's a live problem, though we've seen no related
> trouble reports. It'd require the compiler to generate 8-byte-aligned
> instructions for accessing the ArrayType header, which doesn't seem all
> that probable.
Using different casts, as attached, silences these complaints without much
change in code cosmetics. DatumGetAnyArrayP() already assumes one can cast
between AnyArrayType and ArrayType, so this doesn't introduce new assumptions.
This is enough to make my infrequent "gcc -fsanitize=undefined
-fno-sanitize=nonnull-attribute" runs complaint-free under gcc 8.3.0 on
sparc64. (I use -fno-sanitize=nonnull-attribute to avoid the complaints about
memcpy(NULL, NULL, 0) and such. While formally undefined, fixes for those
would be uglier, and I expect we'll learn quickly enough if some
implementation treats them as other than a no-op.)
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 3ae1556..f9f621e 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -4159,7 +4159,7 @@ array_contain_compare(AnyArrayType *array1, AnyArrayType *array2, Oid collation,
nelems2 = array2->xpn.nelems;
}
else
- deconstruct_array(&(array2->flt),
+ deconstruct_array((ArrayType *) array2,
element_type, typlen, typbyval, typalign,
&values2, &nulls2, &nelems2);
diff --git a/src/include/utils/array.h b/src/include/utils/array.h
index 7c3f947..b441eb4 100644
--- a/src/include/utils/array.h
+++ b/src/include/utils/array.h
@@ -157,7 +157,10 @@ typedef struct ExpandedArrayHeader
/*
* Functions that can handle either a "flat" varlena array or an expanded
- * array use this union to work with their input.
+ * array use this union to work with their input. Don't refer to "flt";
+ * instead, cast to ArrayType. This struct nominally requires 8-byte
+ * alignment on 64-bit, but it's often used for an ArrayType having 4-byte
+ * alignment. UBSan complains about referencing "flt" in such cases.
*/
typedef union AnyArrayType
{
@@ -311,17 +314,21 @@ typedef struct ArrayIteratorData *ArrayIterator;
* Macros for working with AnyArrayType inputs. Beware multiple references!
*/
#define AARR_NDIM(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.ndims : ARR_NDIM(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.ndims : ARR_NDIM((ArrayType *) (a)))
#define AARR_HASNULL(a) \
(VARATT_IS_EXPANDED_HEADER(a) ? \
((a)->xpn.dvalues != NULL ? (a)->xpn.dnulls != NULL : ARR_HASNULL((a)->xpn.fvalue)) : \
- ARR_HASNULL(&(a)->flt))
+ ARR_HASNULL((ArrayType *) (a)))
#define AARR_ELEMTYPE(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.element_type : ARR_ELEMTYPE(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.element_type : ARR_ELEMTYPE((ArrayType *) (a)))
#define AARR_DIMS(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.dims : ARR_DIMS(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.dims : ARR_DIMS((ArrayType *) (a)))
#define AARR_LBOUND(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.lbound : ARR_LBOUND(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.lbound : ARR_LBOUND((ArrayType *) (a)))
/*
diff --git a/src/include/utils/arrayaccess.h b/src/include/utils/arrayaccess.h
index 0917805..47e7426 100644
--- a/src/include/utils/arrayaccess.h
+++ b/src/include/utils/arrayaccess.h
@@ -71,8 +71,8 @@ array_iter_setup(array_iter *it, AnyArrayType *a)
{
it->datumptr = NULL;
it->isnullptr = NULL;
- it->dataptr = ARR_DATA_PTR(&a->flt);
- it->bitmapptr = ARR_NULLBITMAP(&a->flt);
+ it->dataptr = ARR_DATA_PTR((ArrayType *) a);
+ it->bitmapptr = ARR_NULLBITMAP((ArrayType *) a);
}
it->bitmask = 1;
}
diff --git a/src/include/utils/expandeddatum.h b/src/include/utils/expandeddatum.h
index 12e3ac2..8970fa6 100644
--- a/src/include/utils/expandeddatum.h
+++ b/src/include/utils/expandeddatum.h
@@ -126,7 +126,7 @@ struct ExpandedObjectHeader
*/
#define EOH_HEADER_MAGIC (-1)
#define VARATT_IS_EXPANDED_HEADER(PTR) \
- (((ExpandedObjectHeader *) (PTR))->vl_len_ == EOH_HEADER_MAGIC)
+ (((varattrib_4b *) (PTR))->va_4byte.va_header == EOH_HEADER_MAGIC)
/*
* Generic support functions for expanded objects.
Attachments:
[text/plain] ubsan-alignment-v1.patch (3.3K, ../20190629210334.GA1244217@rfd.leadboat.com/2-ubsan-alignment-v1.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 3ae1556..f9f621e 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -4159,7 +4159,7 @@ array_contain_compare(AnyArrayType *array1, AnyArrayType *array2, Oid collation,
nelems2 = array2->xpn.nelems;
}
else
- deconstruct_array(&(array2->flt),
+ deconstruct_array((ArrayType *) array2,
element_type, typlen, typbyval, typalign,
&values2, &nulls2, &nelems2);
diff --git a/src/include/utils/array.h b/src/include/utils/array.h
index 7c3f947..b441eb4 100644
--- a/src/include/utils/array.h
+++ b/src/include/utils/array.h
@@ -157,7 +157,10 @@ typedef struct ExpandedArrayHeader
/*
* Functions that can handle either a "flat" varlena array or an expanded
- * array use this union to work with their input.
+ * array use this union to work with their input. Don't refer to "flt";
+ * instead, cast to ArrayType. This struct nominally requires 8-byte
+ * alignment on 64-bit, but it's often used for an ArrayType having 4-byte
+ * alignment. UBSan complains about referencing "flt" in such cases.
*/
typedef union AnyArrayType
{
@@ -311,17 +314,21 @@ typedef struct ArrayIteratorData *ArrayIterator;
* Macros for working with AnyArrayType inputs. Beware multiple references!
*/
#define AARR_NDIM(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.ndims : ARR_NDIM(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.ndims : ARR_NDIM((ArrayType *) (a)))
#define AARR_HASNULL(a) \
(VARATT_IS_EXPANDED_HEADER(a) ? \
((a)->xpn.dvalues != NULL ? (a)->xpn.dnulls != NULL : ARR_HASNULL((a)->xpn.fvalue)) : \
- ARR_HASNULL(&(a)->flt))
+ ARR_HASNULL((ArrayType *) (a)))
#define AARR_ELEMTYPE(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.element_type : ARR_ELEMTYPE(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.element_type : ARR_ELEMTYPE((ArrayType *) (a)))
#define AARR_DIMS(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.dims : ARR_DIMS(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.dims : ARR_DIMS((ArrayType *) (a)))
#define AARR_LBOUND(a) \
- (VARATT_IS_EXPANDED_HEADER(a) ? (a)->xpn.lbound : ARR_LBOUND(&(a)->flt))
+ (VARATT_IS_EXPANDED_HEADER(a) ? \
+ (a)->xpn.lbound : ARR_LBOUND((ArrayType *) (a)))
/*
diff --git a/src/include/utils/arrayaccess.h b/src/include/utils/arrayaccess.h
index 0917805..47e7426 100644
--- a/src/include/utils/arrayaccess.h
+++ b/src/include/utils/arrayaccess.h
@@ -71,8 +71,8 @@ array_iter_setup(array_iter *it, AnyArrayType *a)
{
it->datumptr = NULL;
it->isnullptr = NULL;
- it->dataptr = ARR_DATA_PTR(&a->flt);
- it->bitmapptr = ARR_NULLBITMAP(&a->flt);
+ it->dataptr = ARR_DATA_PTR((ArrayType *) a);
+ it->bitmapptr = ARR_NULLBITMAP((ArrayType *) a);
}
it->bitmask = 1;
}
diff --git a/src/include/utils/expandeddatum.h b/src/include/utils/expandeddatum.h
index 12e3ac2..8970fa6 100644
--- a/src/include/utils/expandeddatum.h
+++ b/src/include/utils/expandeddatum.h
@@ -126,7 +126,7 @@ struct ExpandedObjectHeader
*/
#define EOH_HEADER_MAGIC (-1)
#define VARATT_IS_EXPANDED_HEADER(PTR) \
- (((ExpandedObjectHeader *) (PTR))->vl_len_ == EOH_HEADER_MAGIC)
+ (((varattrib_4b *) (PTR))->va_4byte.va_header == EOH_HEADER_MAGIC)
/*
* Generic support functions for expanded objects.
view thread (20+ messages) latest in thread
Message-ID: <20190629210334.GA1244217@rfd.leadboat.com>
Permalink: ../20190629210334.GA1244217@rfd.leadboat.com/
Also on: postgresql.org/message-id/20190629210334.GA1244217@rfd.leadboat.com
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: noah@leadboat.com, tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, marxin.liska@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15121: Multiple UBSAN errors
In-Reply-To: <20190629210334.GA1244217@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox