Re: Fix runtime errors from -fsanitize=undefined
On 2019-07-05 19:10, Tom Lane wrote: > Peter Eisentraut 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
Re: Fix runtime errors from -fsanitize=undefined
Peter Eisentraut 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
Re: Fix runtime errors from -fsanitize=undefined
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".
Re: Fix runtime errors from -fsanitize=undefined
> 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
Re: Fix runtime errors from -fsanitize=undefined
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
Re: Fix runtime errors from -fsanitize=undefined
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 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. 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.
Re: Fix runtime errors from -fsanitize=undefined
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 wrote: > > On 2019-06-05 21:30, Robert Haas wrote: > > On Mon, Jun 3, 2019 at 3:22 PM 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 kind > > > > Is 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 > >
Re: Fix runtime errors from -fsanitize=undefined
On 2019-06-05 21:30, Robert Haas wrote: > On Mon, Jun 3, 2019 at 3:22 PM 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 kind > > Is 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
Re: Fix runtime errors from -fsanitize=undefined
On Mon, Jun 3, 2019 at 3:22 PM 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 kind Is this as of some particular gcc version? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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 From 4b657d1af4a8518218be207024b123cfa6d799d8 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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(, 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([i], s->childXids, - s->nChildXids * sizeof(TransactionId)); + if (s->nChildXids > 0) + memcpy([i], s->childXids, + s->nChildXids * sizeof(TransactionId)); i += s->nChildXids; }