From: Tom Lane Date: Thu, 3 Mar 2022 23:13:24 +0000 (-0500) Subject: Clean up assorted failures under clang's -fsanitize=undefined checks. X-Git-Tag: REL_15_BETA1~603 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=46ab07ffda9d6c8e63360ded2d4568aa160a7700;p=postgresql.git Clean up assorted failures under clang's -fsanitize=undefined checks. Most of these are cases where we could call memcpy() or other libc functions with a NULL pointer and a zero count, which is forbidden by POSIX even though every production version of libc allows it. We've fixed such things before in a piecemeal way, but apparently never made an effort to try to get them all. I don't claim that this patch does so either, but it gets every failure I observe in check-world, using clang 12.0.1 on current RHEL8. numeric.c has a different issue that the sanitizer doesn't like: "ln(-1.0)" will compute log10(0) and then try to assign the resulting -Inf to an integer variable. We don't actually use the result in such a case, so there's no live bug. Back-patch to all supported branches, with the idea that we might start running a buildfarm member that tests this case. This includes back-patching c1132aae3 (Check the size in COPY_POINTER_FIELD), which previously silenced some of these issues in copyfuncs.c. Discussion: https://postgr.es/m/CALNJ-vT9r0DSsAOw9OXVJFxLENoVS_68kJ5x0p44atoYH+H4dg@mail.gmail.com --- diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c index 75e2426e9fd..0010addaf7d 100644 --- a/contrib/pgcrypto/px.c +++ b/contrib/pgcrypto/px.c @@ -198,7 +198,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen, ivbuf = palloc0(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 59d43e2ba92..4e6aeba3154 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -328,7 +328,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 > 0) 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 ceadac70d57..ff0b8a688de 100644 --- a/src/backend/access/heap/heapam_visibility.c +++ b/src/backend/access/heap/heapam_visibility.c @@ -1564,8 +1564,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple) static bool TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num) { - return bsearch(&xid, xip, num, - sizeof(TransactionId), xidComparator) != NULL; + return num > 0 && + 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 de787c3d375..3d9088a7048 100644 --- a/src/backend/access/transam/clog.c +++ b/src/backend/access/transam/clog.c @@ -297,8 +297,9 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids, if (all_xact_same_page && xid == MyProc->xid && nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT && nsubxids == MyProc->subxidStatus.count && - memcmp(subxids, MyProc->subxids.xids, - nsubxids * sizeof(TransactionId)) == 0) + (nsubxids == 0 || + memcmp(subxids, MyProc->subxids.xids, + nsubxids * sizeof(TransactionId)) == 0)) { /* * If we can immediately acquire XactSLRULock, we update the status of diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index adf763a8ea4..8964ddf3ebf 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5353,8 +5353,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/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c index 45b0dfc0620..603cf9b0fa7 100644 --- a/src/backend/storage/ipc/shm_mq.c +++ b/src/backend/storage/ipc/shm_mq.c @@ -773,8 +773,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); - mqh->mqh_partial_bytes += rb; + if (rb > 0) + { + memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb); + mqh->mqh_partial_bytes += rb; + } /* * Update count of bytes that can be consumed, accounting for diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index 975d7dcf476..45547f6ae7f 100644 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -10048,12 +10048,20 @@ exp_var(const NumericVar *arg, NumericVar *result, int rscale) * * Essentially, we're approximating log10(abs(ln(var))). This is used to * determine the appropriate rscale when computing natural logarithms. + * + * Note: many callers call this before range-checking the input. Therefore, + * we must be robust against values that are invalid to apply ln() to. + * We don't wish to throw an error here, so just return zero in such cases. */ static int estimate_ln_dweight(const NumericVar *var) { int ln_dweight; + /* Caller should fail on ln(negative), but for the moment return zero */ + if (var->sign != NUMERIC_POS) + return 0; + if (cmp_var(var, &const_zero_point_nine) >= 0 && cmp_var(var, &const_one_point_one) <= 0) { diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index a0b81bf1549..a0be0c411a3 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -536,12 +536,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 2c8e58ebf5c..dcdb2e0d0cb 100644 --- a/src/fe_utils/print.c +++ b/src/fe_utils/print.c @@ -966,7 +966,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)