Date: Sat, 29 Jun 2019 14:03:34 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>, marxin.liska@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15121: Multiple UBSAN errors
Message-ID: <20190629210334.GA1244217@rfd.leadboat.com>
References: <152140318583.11955.7731957058248462341@wrigleys.postgresql.org>
 <7df78014-c931-6c75-6f92-ec48f390b2b9@2ndquadrant.com>
 <20078.1521421379@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="J2SCkAp4GZ/dPZZf"
Content-Disposition: inline
In-Reply-To: <20078.1521421379@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--J2SCkAp4GZ/dPZZf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.)

--J2SCkAp4GZ/dPZZf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="ubsan-alignment-v1.patch"

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.

--J2SCkAp4GZ/dPZZf--