Fix runtime errors from -fsanitize=undefined
After many years of trying, it seems the -fsanitize=undefined checking
in gcc is now working somewhat reliably. Attached is a patch that fixes
all errors of the kind
runtime error: null pointer passed as argument N, which is declared to
never be null
Most of the cases are calls to memcpy(), memcmp(), etc. with a length of
zero, so one appears to get away with passing a null pointer.
Note that these are runtime errors, not static analysis, so the code in
question is actually reached.
To reproduce, configure normally and then set
COPT=-fsanitize=undefined -fno-sanitize=alignment -fno-sanitize-recover=all
and build and run make check-world. Unpatched, this will core dump in
various places.
(-fno-sanitize=alignment should also be fixed but I took it out here to
deal with it separately.)
See https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html for
further documentation.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-runtime-errors-from-fsanitize-undefined.patchtext/plain; charset=UTF-8; name=0001-Fix-runtime-errors-from-fsanitize-undefined.patch; x-mac-creator=0; x-mac-type=0Download
From 4b657d1af4a8518218be207024b123cfa6d799d8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 3 Jun 2019 20:55:35 +0200
Subject: [PATCH] Fix runtime errors from -fsanitize-undefined
These are of the form
runtime error: null pointer passed as argument N, which is declared to never be null
---
contrib/pgcrypto/px.c | 2 +-
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/heap/heapam_visibility.c | 2 ++
src/backend/access/transam/clog.c | 1 +
src/backend/access/transam/xact.c | 5 +++--
src/backend/commands/indexcmds.c | 3 ++-
src/backend/utils/cache/relcache.c | 8 ++++++--
src/backend/utils/misc/guc.c | 2 +-
src/backend/utils/time/snapmgr.c | 10 ++++++----
src/fe_utils/print.c | 3 ++-
10 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 0f02fb56c4..ec9cf99086 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -200,7 +200,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 419da8784a..52e3782282 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -307,7 +307,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
/*
* copy the scan key, if appropriate
*/
- if (key != NULL)
+ if (key != NULL && scan->rs_base.rs_nkeys)
memcpy(scan->rs_base.rs_key, key, scan->rs_base.rs_nkeys * sizeof(ScanKeyData));
/*
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 537e681b23..9c291cd1ab 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -1520,6 +1520,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
static bool
TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
{
+ if (!xip)
+ return false;
return bsearch(&xid, xip, num,
sizeof(TransactionId), xidComparator) != NULL;
}
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 47db7a8a88..4324229a61 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -294,6 +294,7 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
* on the same page. Check those conditions, too.
*/
if (all_xact_same_page && xid == MyPgXact->xid &&
+ nsubxids > 0 &&
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
nsubxids == MyPgXact->nxids &&
memcmp(subxids, MyProc->subxids.xids,
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index f1108ccc8b..976ab5a950 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5201,8 +5201,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
{
if (FullTransactionIdIsValid(s->fullTransactionId))
workspace[i++] = XidFromFullTransactionId(s->fullTransactionId);
- 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/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 40ea629ffe..af45e44a00 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1051,7 +1051,8 @@ DefineIndex(Oid relationId,
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
nparts);
- memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ if (nparts > 0)
+ memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2b992d7832..a56f2a4145 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5900,10 +5900,14 @@ write_relcache_init_file(bool shared)
static void
write_item(const void *data, Size len, FILE *fp)
{
+ Assert(data || len == 0);
if (fwrite(&len, 1, sizeof(len), fp) != sizeof(len))
elog(FATAL, "could not write init file");
- if (fwrite(data, 1, len, fp) != len)
- elog(FATAL, "could not write init file");
+ if (data)
+ {
+ if (fwrite(data, 1, len, fp) != len)
+ elog(FATAL, "could not write init file");
+ }
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1208eb9a68..29b15fa2bd 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9066,7 +9066,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
values[4] = _(conf->short_desc);
/* extra_desc */
- values[5] = _(conf->long_desc);
+ values[5] = conf->long_desc ? _(conf->long_desc) : NULL;
/* context */
values[6] = GucContext_Names[conf->context];
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index ef9fc15ac3..b9df5e8ae6 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -594,12 +594,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, VirtualTransactionId *sourcevxid,
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/fe_utils/print.c b/src/fe_utils/print.c
index e41f42ea98..c7c101aa13 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -913,7 +913,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 (header_done)
+ memset(header_done, false, col_count * sizeof(bool));
while (more_col_wrapping)
{
if (opt_border == 2)
--
2.21.0
On Mon, Jun 3, 2019 at 3:22 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
After many years of trying, it seems the -fsanitize=undefined checking
in gcc is now working somewhat reliably. Attached is a patch that fixes
all errors of the kind
Is this as of some particular gcc version?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2019-06-05 21:30, Robert Haas wrote:
On Mon, Jun 3, 2019 at 3:22 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:After many years of trying, it seems the -fsanitize=undefined checking
in gcc is now working somewhat reliably. Attached is a patch that fixes
all errors of the kindIs this as of some particular gcc version?
I used gcc-8.
The option has existed in gcc for quite some time, but in previous
releases it always tended to hang or get confused somewhere.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
I tested this patch with clang 7 on master.
- On unpatched master I can't reproduce errors with make check-world in:
src/backend/access/heap/heapam.c
src/backend/utils/cache/relcache.c (IIRC I triggered this one in a pg
previous version)
src/backend/utils/misc/guc.c
- I have a hard to reproduce one not in this patched:
src/backend/storage/ipc/shm_mq.c line 727
About the changes
- in
src/fe_utils/print.c
line memset(header_done, false, col_count * sizeof(bool));
is redundant and should be remove not guarded with if (hearder_done),
header_done is either null or already zeroed, it's pg_malloc0 ed.
In all cases but one patched version shortcut an undefined no ops but in
src/backend/access/transam/clog.c
memcmp 0 bytes return 0 thus current change modifies code path, before
with nsubxids == 0 if branch was taken now it's not.
Could wait more often while taking lock, no idea if it's relevant.
Regards
Didier
On Thu, Jun 6, 2019 at 11:36 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
Show quoted text
On 2019-06-05 21:30, Robert Haas wrote:
On Mon, Jun 3, 2019 at 3:22 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:After many years of trying, it seems the -fsanitize=undefined checking
in gcc is now working somewhat reliably. Attached is a patch that fixes
all errors of the kindIs this as of some particular gcc version?
I used gcc-8.
The option has existed in gcc for quite some time, but in previous
releases it always tended to hang or get confused somewhere.--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jun 03, 2019 at 09:21:48PM +0200, Peter Eisentraut wrote:
After many years of trying, it seems the -fsanitize=undefined checking
in gcc is now working somewhat reliably. Attached is a patch that fixes
all errors of the kindruntime error: null pointer passed as argument N, which is declared to
never be nullMost of the cases are calls to memcpy(), memcmp(), etc. with a length of
zero, so one appears to get away with passing a null pointer.
I just saw this proposal. The undefined behavior in question is strictly
academic. These changes do remove the need for new users to discover
-fno-sanitize=nonnull-attribute, but they make the code longer and no clearer.
Given the variety of code this touches, I expect future commits will
reintroduce the complained-of usage patterns, prompting yet more commits to
restore the invariant achieved here. Hence, I'm -0 for this change.
On 2019-07-05 01:33, Noah Misch wrote:
I just saw this proposal. The undefined behavior in question is strictly
academic. These changes do remove the need for new users to discover
-fno-sanitize=nonnull-attribute, but they make the code longer and no clearer.
Given the variety of code this touches, I expect future commits will
reintroduce the complained-of usage patterns, prompting yet more commits to
restore the invariant achieved here. Hence, I'm -0 for this change.
This sanitizer has found real problems in the past. By removing these
trivial issues we can then set up a build farm animal or similar to
automatically check for any new issues.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
This sanitizer has found real problems in the past. By removing these
trivial issues we can then set up a build farm animal or similar to
automatically check for any new issues.
We have done exactly this in postgis with 2 different jobs (gcc and clang)
and, even though it doesn't happen too often, it's really satisfying when
it detects these issues automatically.
--
Raúl Marín Rodríguez
carto.com
On Fri, Jul 05, 2019 at 06:14:31PM +0200, Peter Eisentraut wrote:
On 2019-07-05 01:33, Noah Misch wrote:
I just saw this proposal. The undefined behavior in question is strictly
academic. These changes do remove the need for new users to discover
-fno-sanitize=nonnull-attribute, but they make the code longer and no clearer.
Given the variety of code this touches, I expect future commits will
reintroduce the complained-of usage patterns, prompting yet more commits to
restore the invariant achieved here. Hence, I'm -0 for this change.This sanitizer has found real problems in the past. By removing these
trivial issues we can then set up a build farm animal or similar to
automatically check for any new issues.
Has it found one real problem that it would not have found given
"-fno-sanitize=nonnull-attribute"? I like UBSan in general, but I haven't
found a reason to prefer plain "-fsanitize=undefined" over
"-fsanitize=undefined -fno-sanitize=nonnull-attribute".
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 2019-07-05 01:33, Noah Misch wrote:
I just saw this proposal. The undefined behavior in question is strictly
academic. These changes do remove the need for new users to discover
-fno-sanitize=nonnull-attribute, but they make the code longer and no clearer.
Given the variety of code this touches, I expect future commits will
reintroduce the complained-of usage patterns, prompting yet more commits to
restore the invariant achieved here. Hence, I'm -0 for this change.
This sanitizer has found real problems in the past. By removing these
trivial issues we can then set up a build farm animal or similar to
automatically check for any new issues.
I think Noah's point is just that we can do that with the addition of
-fno-sanitize=nonnull-attribute. I agree with him that it's very
unclear why we should bother to make the code clean against that
specific subset of warnings.
regards, tom lane
On 2019-07-05 19:10, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 2019-07-05 01:33, Noah Misch wrote:
I just saw this proposal. The undefined behavior in question is strictly
academic. These changes do remove the need for new users to discover
-fno-sanitize=nonnull-attribute, but they make the code longer and no clearer.
Given the variety of code this touches, I expect future commits will
reintroduce the complained-of usage patterns, prompting yet more commits to
restore the invariant achieved here. Hence, I'm -0 for this change.This sanitizer has found real problems in the past. By removing these
trivial issues we can then set up a build farm animal or similar to
automatically check for any new issues.I think Noah's point is just that we can do that with the addition of
-fno-sanitize=nonnull-attribute. I agree with him that it's very
unclear why we should bother to make the code clean against that
specific subset of warnings.
OK, I'm withdrawing this patch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services