Skip to content

Commit b0bc196

Browse files
tglsfdctglsfdc
committed
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 c1132aa (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
1 parent 2a1f846 commit b0bc196

File tree

10 files changed

+52
-48
lines changed

10 files changed

+52
-48
lines changed

‎contrib/pgcrypto/px.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
200200
ivbuf = palloc0(ivs);
201201
if (ivlen > ivs)
202202
memcpy(ivbuf, iv, ivs);
203-
else
203+
else if (ivlen > 0)
204204
memcpy(ivbuf, iv, ivlen);
205205
}
206206

‎src/backend/access/heap/heapam.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
328328
/*
329329
* copy the scan key, if appropriate
330330
*/
331-
if (key != NULL)
331+
if (key != NULL && scan->rs_base.rs_nkeys > 0)
332332
memcpy(scan->rs_base.rs_key, key, scan->rs_base.rs_nkeys * sizeof(ScanKeyData));
333333

334334
/*

‎src/backend/access/heap/heapam_visibility.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1564,8 +1564,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
15641564
static bool
15651565
TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
15661566
{
1567-
return bsearch(&xid, xip, num,
1568-
sizeof(TransactionId), xidComparator) != NULL;
1567+
return num > 0 &&
1568+
bsearch(&xid, xip, num, sizeof(TransactionId), xidComparator) != NULL;
15691569
}
15701570

15711571
/*

‎src/backend/access/transam/clog.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,9 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
297297
if (all_xact_same_page && xid == MyProc->xid &&
298298
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
299299
nsubxids == MyProc->subxidStatus.count &&
300-
memcmp(subxids, MyProc->subxids.xids,
301-
nsubxids * sizeof(TransactionId)) == 0)
300+
(nsubxids == 0 ||
301+
memcmp(subxids, MyProc->subxids.xids,
302+
nsubxids * sizeof(TransactionId)) == 0))
302303
{
303304
/*
304305
* If we can immediately acquire XactSLRULock, we update the status of

‎src/backend/access/transam/xact.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5305,8 +5305,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
53055305
{
53065306
if (FullTransactionIdIsValid(s->fullTransactionId))
53075307
workspace[i++] = XidFromFullTransactionId(s->fullTransactionId);
5308-
memcpy(&workspace[i], s->childXids,
5309-
s->nChildXids * sizeof(TransactionId));
5308+
if (s->nChildXids > 0)
5309+
memcpy(&workspace[i], s->childXids,
5310+
s->nChildXids * sizeof(TransactionId));
53105311
i += s->nChildXids;
53115312
}
53125313
Assert(i == nxids);

‎src/backend/nodes/copyfuncs.c

Lines changed: 21 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,11 @@
5757
#define COPY_POINTER_FIELD(fldname, sz) \
5858
do { \
5959
Size _size = (sz); \
60-
newnode->fldname = palloc(_size); \
61-
memcpy(newnode->fldname, from->fldname, _size); \
60+
if (_size > 0) \
61+
{ \
62+
newnode->fldname = palloc(_size); \
63+
memcpy(newnode->fldname, from->fldname, _size); \
64+
} \
6265
} while (0)
6366

6467
/* Copy a parse location field (for Copy, this is same as scalar case) */
@@ -296,12 +299,9 @@ _copyRecursiveUnion(const RecursiveUnion *from)
296299
*/
297300
COPY_SCALAR_FIELD(wtParam);
298301
COPY_SCALAR_FIELD(numCols);
299-
if (from->numCols > 0)
300-
{
301-
COPY_POINTER_FIELD(dupColIdx, from->numCols * sizeof(AttrNumber));
302-
COPY_POINTER_FIELD(dupOperators, from->numCols * sizeof(Oid));
303-
COPY_POINTER_FIELD(dupCollations, from->numCols * sizeof(Oid));
304-
}
302+
COPY_POINTER_FIELD(dupColIdx, from->numCols * sizeof(AttrNumber));
303+
COPY_POINTER_FIELD(dupOperators, from->numCols * sizeof(Oid));
304+
COPY_POINTER_FIELD(dupCollations, from->numCols * sizeof(Oid));
305305
COPY_SCALAR_FIELD(numGroups);
306306

307307
return newnode;
@@ -897,13 +897,10 @@ _copyMergeJoin(const MergeJoin *from)
897897
COPY_SCALAR_FIELD(skip_mark_restore);
898898
COPY_NODE_FIELD(mergeclauses);
899899
numCols = list_length(from->mergeclauses);
900-
if (numCols > 0)
901-
{
902-
COPY_POINTER_FIELD(mergeFamilies, numCols * sizeof(Oid));
903-
COPY_POINTER_FIELD(mergeCollations, numCols * sizeof(Oid));
904-
COPY_POINTER_FIELD(mergeStrategies, numCols * sizeof(int));
905-
COPY_POINTER_FIELD(mergeNullsFirst, numCols * sizeof(bool));
906-
}
900+
COPY_POINTER_FIELD(mergeFamilies, numCols * sizeof(Oid));
901+
COPY_POINTER_FIELD(mergeCollations, numCols * sizeof(Oid));
902+
COPY_POINTER_FIELD(mergeStrategies, numCols * sizeof(int));
903+
COPY_POINTER_FIELD(mergeNullsFirst, numCols * sizeof(bool));
907904

908905
return newnode;
909906
}
@@ -1067,12 +1064,9 @@ _copyAgg(const Agg *from)
10671064
COPY_SCALAR_FIELD(aggstrategy);
10681065
COPY_SCALAR_FIELD(aggsplit);
10691066
COPY_SCALAR_FIELD(numCols);
1070-
if (from->numCols > 0)
1071-
{
1072-
COPY_POINTER_FIELD(grpColIdx, from->numCols * sizeof(AttrNumber));
1073-
COPY_POINTER_FIELD(grpOperators, from->numCols * sizeof(Oid));
1074-
COPY_POINTER_FIELD(grpCollations, from->numCols * sizeof(Oid));
1075-
}
1067+
COPY_POINTER_FIELD(grpColIdx, from->numCols * sizeof(AttrNumber));
1068+
COPY_POINTER_FIELD(grpOperators, from->numCols * sizeof(Oid));
1069+
COPY_POINTER_FIELD(grpCollations, from->numCols * sizeof(Oid));
10761070
COPY_SCALAR_FIELD(numGroups);
10771071
COPY_SCALAR_FIELD(transitionSpace);
10781072
COPY_BITMAPSET_FIELD(aggParams);
@@ -1094,19 +1088,13 @@ _copyWindowAgg(const WindowAgg *from)
10941088

10951089
COPY_SCALAR_FIELD(winref);
10961090
COPY_SCALAR_FIELD(partNumCols);
1097-
if (from->partNumCols > 0)
1098-
{
1099-
COPY_POINTER_FIELD(partColIdx, from->partNumCols * sizeof(AttrNumber));
1100-
COPY_POINTER_FIELD(partOperators, from->partNumCols * sizeof(Oid));
1101-
COPY_POINTER_FIELD(partCollations, from->partNumCols * sizeof(Oid));
1102-
}
1091+
COPY_POINTER_FIELD(partColIdx, from->partNumCols * sizeof(AttrNumber));
1092+
COPY_POINTER_FIELD(partOperators, from->partNumCols * sizeof(Oid));
1093+
COPY_POINTER_FIELD(partCollations, from->partNumCols * sizeof(Oid));
11031094
COPY_SCALAR_FIELD(ordNumCols);
1104-
if (from->ordNumCols > 0)
1105-
{
1106-
COPY_POINTER_FIELD(ordColIdx, from->ordNumCols * sizeof(AttrNumber));
1107-
COPY_POINTER_FIELD(ordOperators, from->ordNumCols * sizeof(Oid));
1108-
COPY_POINTER_FIELD(ordCollations, from->ordNumCols * sizeof(Oid));
1109-
}
1095+
COPY_POINTER_FIELD(ordColIdx, from->ordNumCols * sizeof(AttrNumber));
1096+
COPY_POINTER_FIELD(ordOperators, from->ordNumCols * sizeof(Oid));
1097+
COPY_POINTER_FIELD(ordCollations, from->ordNumCols * sizeof(Oid));
11101098
COPY_SCALAR_FIELD(frameOptions);
11111099
COPY_NODE_FIELD(startOffset);
11121100
COPY_NODE_FIELD(endOffset);

‎src/backend/storage/ipc/shm_mq.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -748,8 +748,11 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
748748

749749
/* Copy as much as we can. */
750750
Assert(mqh->mqh_partial_bytes + rb <= nbytes);
751-
memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
752-
mqh->mqh_partial_bytes += rb;
751+
if (rb > 0)
752+
{
753+
memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
754+
mqh->mqh_partial_bytes += rb;
755+
}
753756

754757
/*
755758
* Update count of bytes that can be consumed, accounting for

‎src/backend/utils/adt/numeric.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9868,12 +9868,20 @@ exp_var(const NumericVar *arg, NumericVar *result, int rscale)
98689868
*
98699869
* Essentially, we're approximating log10(abs(ln(var))). This is used to
98709870
* determine the appropriate rscale when computing natural logarithms.
9871+
*
9872+
* Note: many callers call this before range-checking the input. Therefore,
9873+
* we must be robust against values that are invalid to apply ln() to.
9874+
* We don't wish to throw an error here, so just return zero in such cases.
98719875
*/
98729876
static int
98739877
estimate_ln_dweight(const NumericVar *var)
98749878
{
98759879
int ln_dweight;
98769880

9881+
/* Caller should fail on ln(negative), but for the moment return zero */
9882+
if (var->sign != NUMERIC_POS)
9883+
return 0;
9884+
98779885
if (cmp_var(var, &const_zero_point_nine) >= 0 &&
98789886
cmp_var(var, &const_one_point_one) <= 0)
98799887
{

‎src/backend/utils/time/snapmgr.c

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -536,12 +536,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, VirtualTransactionId *sourcevxid,
536536
CurrentSnapshot->xmax = sourcesnap->xmax;
537537
CurrentSnapshot->xcnt = sourcesnap->xcnt;
538538
Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
539-
memcpy(CurrentSnapshot->xip, sourcesnap->xip,
540-
sourcesnap->xcnt * sizeof(TransactionId));
539+
if (sourcesnap->xcnt > 0)
540+
memcpy(CurrentSnapshot->xip, sourcesnap->xip,
541+
sourcesnap->xcnt * sizeof(TransactionId));
541542
CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
542543
Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
543-
memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
544-
sourcesnap->subxcnt * sizeof(TransactionId));
544+
if (sourcesnap->subxcnt > 0)
545+
memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
546+
sourcesnap->subxcnt * sizeof(TransactionId));
545547
CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
546548
CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
547549
/* NB: curcid should NOT be copied, it's a local matter */

‎src/fe_utils/print.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -896,7 +896,8 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
896896

897897
more_col_wrapping = col_count;
898898
curr_nl_line = 0;
899-
memset(header_done, false, col_count * sizeof(bool));
899+
if (col_count > 0)
900+
memset(header_done, false, col_count * sizeof(bool));
900901
while (more_col_wrapping)
901902
{
902903
if (opt_border == 2)

0 commit comments

Comments
 (0)