Small fix: avoid passing null pointers to memcpy()
Hello,
from running the regression test suite (including TAP tests) and also
sqlsmith, I've got a couple of places where UBSan reported calls to
memcpy() with null pointer passed as either source or destination.
Patch attached.
Attachments:
memcpy-avoid-nullptr.difftext/x-patch; name=memcpy-avoid-nullptr.diffDownload
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index cfb3b50..600824a 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
memset(ivbuf, 0, ivs);
if (ivlen > ivs)
memcpy(ivbuf, iv, ivs);
- else
+ else if (ivlen > 0)
memcpy(ivbuf, iv, ivlen);
}
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 34ba385..67c7586 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
/*
* copy the scan key, if appropriate
*/
- if (key != NULL)
+ if (key != NULL && scan->rs_nkeys > 0)
memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData));
/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 7e37331..e7599be 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4872,8 +4872,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
{
if (TransactionIdIsValid(s->transactionId))
workspace[i++] = s->transactionId;
- memcpy(&workspace[i], s->childXids,
- s->nChildXids * sizeof(TransactionId));
+ if (s->nChildXids > 0)
+ memcpy(&workspace[i], s->childXids,
+ s->nChildXids * sizeof(TransactionId));
i += s->nChildXids;
}
Assert(i == nxids);
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b88e012..dc7a323 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -402,12 +402,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
CurrentSnapshot->xmax = sourcesnap->xmax;
CurrentSnapshot->xcnt = sourcesnap->xcnt;
Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
- memcpy(CurrentSnapshot->xip, sourcesnap->xip,
- sourcesnap->xcnt * sizeof(TransactionId));
+ if (sourcesnap->xcnt > 0)
+ memcpy(CurrentSnapshot->xip, sourcesnap->xip,
+ sourcesnap->xcnt * sizeof(TransactionId));
CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
- memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
- sourcesnap->subxcnt * sizeof(TransactionId));
+ if (sourcesnap->subxcnt > 0)
+ memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
+ sourcesnap->subxcnt * sizeof(TransactionId));
CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
/* NB: curcid should NOT be copied, it's a local matter */
On 2016-04-03 09:24, Piotr Stefaniak wrote:
from running the regression test suite (including TAP tests) and also
sqlsmith, I've got a couple of places where UBSan reported calls to
memcpy() with null pointer passed as either source or destination.Patch attached.
Patch updated.
Since this time the patch includes fixes for other standard library
function calls (memset and bsearch), I'm renaming the patch file to be
more generic.
Attachments:
dont-pass-null-pointers-ub.patchtext/x-patch; name=dont-pass-null-pointers-ub.patchDownload
commit 75e849e83c8f7d6b4caab13271b7b0fcf124d497
Author: Piotr Stefaniak <postgres@piotr-stefaniak.me>
Date: Sat Apr 16 14:28:34 2016 +0200
Don't pass null pointers to functions that require the pointers to be non null.
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index cfb3b50..600824a 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
memset(ivbuf, 0, ivs);
if (ivlen > ivs)
memcpy(ivbuf, iv, ivs);
- else
+ else if (ivlen > 0)
memcpy(ivbuf, iv, ivlen);
}
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 29fd31a..2ed56ab 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
/*
* copy the scan key, if appropriate
*/
- if (key != NULL)
+ if (key != NULL && scan->rs_nkeys > 0)
memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData));
/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 7e37331..e7599be 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4872,8 +4872,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
{
if (TransactionIdIsValid(s->transactionId))
workspace[i++] = s->transactionId;
- memcpy(&workspace[i], s->childXids,
- s->nChildXids * sizeof(TransactionId));
+ if (s->nChildXids > 0)
+ memcpy(&workspace[i], s->childXids,
+ s->nChildXids * sizeof(TransactionId));
i += s->nChildXids;
}
Assert(i == nxids);
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index b4dc617..a72795c 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1563,9 +1563,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
/* copy running xacts */
sz = sizeof(TransactionId) * builder->running.xcnt_space;
- memcpy(ondisk_c, builder->running.xip, sz);
- COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
- ondisk_c += sz;
+ if (sz > 0)
+ {
+ memcpy(ondisk_c, builder->running.xip, sz);
+ COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
+ ondisk_c += sz;
+ }
/* copy committed xacts */
sz = sizeof(TransactionId) * builder->committed.xcnt;
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 8aa1f49..25ac26f 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -506,12 +506,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
CurrentSnapshot->xmax = sourcesnap->xmax;
CurrentSnapshot->xcnt = sourcesnap->xcnt;
Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
- memcpy(CurrentSnapshot->xip, sourcesnap->xip,
- sourcesnap->xcnt * sizeof(TransactionId));
+ if (sourcesnap->xcnt > 0)
+ memcpy(CurrentSnapshot->xip, sourcesnap->xip,
+ sourcesnap->xcnt * sizeof(TransactionId));
CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
- memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
- sourcesnap->subxcnt * sizeof(TransactionId));
+ if (sourcesnap->subxcnt > 0)
+ memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
+ sourcesnap->subxcnt * sizeof(TransactionId));
CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
/* NB: curcid should NOT be copied, it's a local matter */
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 0e815a9..0f26bd8 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -1639,6 +1639,7 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
static bool
TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
{
+ Assert(xip != NULL);
return bsearch(&xid, xip, num,
sizeof(TransactionId), xidComparator) != NULL;
}
@@ -1675,7 +1676,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
return false;
}
/* check if it's one of our txids, toplevel is also in there */
- else if (TransactionIdInArray(xmin, snapshot->subxip, snapshot->subxcnt))
+ else if (snapshot->subxcnt > 0 && TransactionIdInArray(xmin, snapshot->subxip, snapshot->subxcnt))
{
bool resolved;
CommandId cmin = HeapTupleHeaderGetRawCommandId(tuple);
@@ -1717,7 +1718,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
return false;
}
/* check if it's a committed transaction in [xmin, xmax) */
- else if (TransactionIdInArray(xmin, snapshot->xip, snapshot->xcnt))
+ else if (snapshot->xcnt > 0 && TransactionIdInArray(xmin, snapshot->xip, snapshot->xcnt))
{
/* fall through */
}
@@ -1750,7 +1751,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
}
/* check if it's one of our txids, toplevel is also in there */
- if (TransactionIdInArray(xmax, snapshot->subxip, snapshot->subxcnt))
+ if (snapshot->subxcnt > 0 && TransactionIdInArray(xmax, snapshot->subxip, snapshot->subxcnt))
{
bool resolved;
CommandId cmin;
@@ -1788,7 +1789,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
else if (TransactionIdFollowsOrEquals(xmax, snapshot->xmax))
return true;
/* xmax is between [xmin, xmax), check known committed array */
- else if (TransactionIdInArray(xmax, snapshot->xip, snapshot->xcnt))
+ else if (snapshot->xcnt > 0 && TransactionIdInArray(xmax, snapshot->xip, snapshot->xcnt))
return false;
/* xmax is between [xmin, xmax), but known not to have committed yet */
else
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 1ec74f1..ca0eb7e 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -914,7 +914,8 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
more_col_wrapping = col_count;
curr_nl_line = 0;
- memset(header_done, false, col_count * sizeof(bool));
+ if (col_count > 0)
+ memset(header_done, false, col_count * sizeof(bool));
while (more_col_wrapping)
{
if (opt_border == 2)
Added a fix for src/backend/storage/ipc/shm_mq.c:shm_mq_receive.
Attachments:
dont_pass_null_pointers_ub_v2.patchtext/x-patch; name=dont_pass_null_pointers_ub_v2.patchDownload
commit 936c7268b61460deb201b9e6bbfb60ab5258ec87
Author: Piotr Stefaniak <postgres@piotr-stefaniak.me>
Date: Thu Apr 28 18:35:43 2016 +0200
Don't pass null pointers to functions that require the pointers to be non null.
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index cfb3b50..600824a 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
memset(ivbuf, 0, ivs);
if (ivlen > ivs)
memcpy(ivbuf, iv, ivs);
- else
+ else if (ivlen > 0)
memcpy(ivbuf, iv, ivlen);
}
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 950bfc8..bf9a7ab 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
/*
* copy the scan key, if appropriate
*/
- if (key != NULL)
+ if (key != NULL && scan->rs_nkeys > 0)
memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData));
/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 95690ff..4ae98e6 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4890,8 +4890,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
{
if (TransactionIdIsValid(s->transactionId))
workspace[i++] = s->transactionId;
- memcpy(&workspace[i], s->childXids,
- s->nChildXids * sizeof(TransactionId));
+ if (s->nChildXids > 0)
+ memcpy(&workspace[i], s->childXids,
+ s->nChildXids * sizeof(TransactionId));
i += s->nChildXids;
}
Assert(i == nxids);
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index b4dc617..a72795c 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1563,9 +1563,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
/* copy running xacts */
sz = sizeof(TransactionId) * builder->running.xcnt_space;
- memcpy(ondisk_c, builder->running.xip, sz);
- COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
- ondisk_c += sz;
+ if (sz > 0)
+ {
+ memcpy(ondisk_c, builder->running.xip, sz);
+ COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
+ ondisk_c += sz;
+ }
/* copy committed xacts */
sz = sizeof(TransactionId) * builder->committed.xcnt;
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 7d1c9cd..7859f42 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -492,7 +492,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
shm_mq_result res;
Size rb = 0;
Size nbytes;
- void *rawdata;
+ void *rawdata = NULL;
Assert(mq->mq_receiver == MyProc);
@@ -673,7 +673,11 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
/* Copy as much as we can. */
Assert(mqh->mqh_partial_bytes + rb <= nbytes);
- memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
+ if (rb > 0)
+ {
+ Assert(rawdata != NULL);
+ memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
+ }
mqh->mqh_partial_bytes += rb;
/*
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 0a9a231..ff09171 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -506,12 +506,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
CurrentSnapshot->xmax = sourcesnap->xmax;
CurrentSnapshot->xcnt = sourcesnap->xcnt;
Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
- memcpy(CurrentSnapshot->xip, sourcesnap->xip,
- sourcesnap->xcnt * sizeof(TransactionId));
+ if (sourcesnap->xcnt > 0)
+ memcpy(CurrentSnapshot->xip, sourcesnap->xip,
+ sourcesnap->xcnt * sizeof(TransactionId));
CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
- memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
- sourcesnap->subxcnt * sizeof(TransactionId));
+ if (sourcesnap->subxcnt > 0)
+ memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
+ sourcesnap->subxcnt * sizeof(TransactionId));
CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
/* NB: curcid should NOT be copied, it's a local matter */
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 0e815a9..0f26bd8 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -1639,6 +1639,7 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
static bool
TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
{
+ Assert(xip != NULL);
return bsearch(&xid, xip, num,
sizeof(TransactionId), xidComparator) != NULL;
}
@@ -1675,7 +1676,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
return false;
}
/* check if it's one of our txids, toplevel is also in there */
- else if (TransactionIdInArray(xmin, snapshot->subxip, snapshot->subxcnt))
+ else if (snapshot->subxcnt > 0 && TransactionIdInArray(xmin, snapshot->subxip, snapshot->subxcnt))
{
bool resolved;
CommandId cmin = HeapTupleHeaderGetRawCommandId(tuple);
@@ -1717,7 +1718,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
return false;
}
/* check if it's a committed transaction in [xmin, xmax) */
- else if (TransactionIdInArray(xmin, snapshot->xip, snapshot->xcnt))
+ else if (snapshot->xcnt > 0 && TransactionIdInArray(xmin, snapshot->xip, snapshot->xcnt))
{
/* fall through */
}
@@ -1750,7 +1751,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
}
/* check if it's one of our txids, toplevel is also in there */
- if (TransactionIdInArray(xmax, snapshot->subxip, snapshot->subxcnt))
+ if (snapshot->subxcnt > 0 && TransactionIdInArray(xmax, snapshot->subxip, snapshot->subxcnt))
{
bool resolved;
CommandId cmin;
@@ -1788,7 +1789,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
else if (TransactionIdFollowsOrEquals(xmax, snapshot->xmax))
return true;
/* xmax is between [xmin, xmax), check known committed array */
- else if (TransactionIdInArray(xmax, snapshot->xip, snapshot->xcnt))
+ else if (snapshot->xcnt > 0 && TransactionIdInArray(xmax, snapshot->xip, snapshot->xcnt))
return false;
/* xmax is between [xmin, xmax), but known not to have committed yet */
else
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 1ec74f1..ca0eb7e 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -914,7 +914,8 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
more_col_wrapping = col_count;
curr_nl_line = 0;
- memset(header_done, false, col_count * sizeof(bool));
+ if (col_count > 0)
+ memset(header_done, false, col_count * sizeof(bool));
while (more_col_wrapping)
{
if (opt_border == 2)
On 4/16/16 8:48 AM, Piotr Stefaniak wrote:
On 2016-04-03 09:24, Piotr Stefaniak wrote:
from running the regression test suite (including TAP tests) and also
sqlsmith, I've got a couple of places where UBSan reported calls to
memcpy() with null pointer passed as either source or destination.Patch attached.
Patch updated.
Since this time the patch includes fixes for other standard library
function calls (memset and bsearch), I'm renaming the patch file to be
more generic.
Most of these changes appear to address the case where the size argument
of memcpy() is zero. I'm not sure why those changes are necessary. At
least in some cases that I sporadically checked, a zero size does not
lead to a null pointer argument. We'll need some more details here.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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
Hi
A smaller version removing memset in print_aligned_text function.
The line is redundant , header_done isn't used yet and it's either
pg_malloc0 or null.
Without this patch make check fails 3 tests if pg is compiled with
-fsanitize=address,undefined
Attachments:
Remove_redundant_memset_v1.patchtext/x-patch; charset=US-ASCII; name=Remove_redundant_memset_v1.patchDownload
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index cb9a9a0613..b13167859c 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -913,7 +913,6 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
more_col_wrapping = col_count;
curr_nl_line = 0;
- memset(header_done, false, col_count * sizeof(bool));
while (more_col_wrapping)
{
if (opt_border == 2)
didier <did447@gmail.com> writes:
A smaller version removing memset in print_aligned_text function.
The line is redundant , header_done isn't used yet and it's either
pg_malloc0 or null.
Hm, I see the theoretical problem ...
Without this patch make check fails 3 tests if pg is compiled with
-fsanitize=address,undefined
... but if that's the only evidence of an actual problem, I can't
get excited about it. ASAN complains about many things in Postgres,
and most of them are pretty hypothetical.
regards, tom lane
Hi,
On Fri, May 24, 2019 at 5:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
didier <did447@gmail.com> writes:
A smaller version removing memset in print_aligned_text function.
The line is redundant , header_done isn't used yet and it's either
pg_malloc0 or null.Hm, I see the theoretical problem ...
Without this patch make check fails 3 tests if pg is compiled with
-fsanitize=address,undefined... but if that's the only evidence of an actual problem, I can't
get excited about it. ASAN complains about many things in Postgres,
Not that much, make check has only this one.
and most of them are pretty hypothetical.
Well there's no point for this line even without ASAN, why call memset
on header_done but not on width_header,
width_average, and so on?
ASAN complaining at runtime because memset is called with a null ptr
and a zero count is just the nudge for removing it.
Regards
Didier
I wrote:
didier <did447@gmail.com> writes:
Without this patch make check fails 3 tests if pg is compiled with
-fsanitize=address,undefined
... but if that's the only evidence of an actual problem, I can't
get excited about it. ASAN complains about many things in Postgres,
and most of them are pretty hypothetical.
BTW, I did try the described test case, so I suppose I might as well
summarize the results while I have them. The postmaster log from
a regression run with today's HEAD shows ASAN errors at
clog.c:299:3:
indexcmds.c:1054:4:
relcache.c:5905:6:
snapmgr.c:597:2:
snapmgr.c:601:2:
xact.c:5204:3:
The above all seem to be the same ilk as the problem in print.c,
ie passing a NULL pointer with zero count to memcpy, memset, or
the like. At least some of them are fairly old.
pg_crc32c_sse42.c:37:18:
pg_crc32c_sse42.c:44:9:
This is an intentional use of unaligned access:
* NB: We do unaligned accesses here. The Intel architecture allows that,
* and performance testing didn't show any performance gain from aligning
* the begin address.
This comment is unclear about whether it would actually hurt to have a
preparatory loop to align the begin address.
... and a whole bunch of places in arrayaccess.h and arrayfuncs.c.
These seem to be down to use of AnyArrayType:
typedef union AnyArrayType
{
ArrayType flt;
ExpandedArrayHeader xpn;
} AnyArrayType;
ASAN seems to believe that use of this union entitles the compiler to
assume 8-byte alignment even when touching fields of a short-header
array. Maybe it's right, but this code has been like this for awhile
and we've not heard trouble reports. I'm afraid that avoiding the
use of the union would be a notational mess, so I'm loath to change
it unless we're forced to.
This is just from the core tests, there could well be more issues in
contrib or PLs.
regards, tom lane
Hi,
On Fri, May 24, 2019 at 6:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
These seem to be down to use of AnyArrayType:
typedef union AnyArrayType
{
ArrayType flt;
ExpandedArrayHeader xpn;
} AnyArrayType;ASAN seems to believe that use of this union entitles the compiler to
assume 8-byte alignment even when touching fields of a short-header
In my understanding union has to be aligned to ExpandedArrayHeader (8
bytes) or it's a UB.
On x64 it could be an issue if AnyArrayType alignment is less than 4,
sse is enable and suddenly compiler choose to use sse instructions
with 16 bytes requirement then compiler may not emit the right code.
It's not rhetorical, big surprise first time you get a SIGBUS signal,
or a SIGFPE doing integer math, on x64.
Regards
Didier