Skip to content

Commit 1a027e6

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 6599d8f commit 1a027e6

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
memset(ivbuf, 0, 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
@@ -309,7 +309,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
309309
/*
310310
* copy the scan key, if appropriate
311311
*/
312-
if (key != NULL)
312+
if (key != NULL && scan->rs_base.rs_nkeys > 0)
313313
memcpy(scan->rs_base.rs_key, key, scan->rs_base.rs_nkeys * sizeof(ScanKeyData));
314314

315315
/*

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,8 +1520,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
15201520
static bool
15211521
TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
15221522
{
1523-
return bsearch(&xid, xip, num,
1524-
sizeof(TransactionId), xidComparator) != NULL;
1523+
return num > 0 &&
1524+
bsearch(&xid, xip, num, sizeof(TransactionId), xidComparator) != NULL;
15251525
}
15261526

15271527
/*

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,9 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
296296
if (all_xact_same_page && xid == MyPgXact->xid &&
297297
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
298298
nsubxids == MyPgXact->nxids &&
299-
memcmp(subxids, MyProc->subxids.xids,
300-
nsubxids * sizeof(TransactionId)) == 0)
299+
(nsubxids == 0 ||
300+
memcmp(subxids, MyProc->subxids.xids,
301+
nsubxids * sizeof(TransactionId)) == 0))
301302
{
302303
/*
303304
* 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
@@ -5282,8 +5282,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
52825282
{
52835283
if (FullTransactionIdIsValid(s->fullTransactionId))
52845284
workspace[i++] = XidFromFullTransactionId(s->fullTransactionId);
5285-
memcpy(&workspace[i], s->childXids,
5286-
s->nChildXids * sizeof(TransactionId));
5285+
if (s->nChildXids > 0)
5286+
memcpy(&workspace[i], s->childXids,
5287+
s->nChildXids * sizeof(TransactionId));
52875288
i += s->nChildXids;
52885289
}
52895290
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;
@@ -875,13 +875,10 @@ _copyMergeJoin(const MergeJoin *from)
875875
COPY_SCALAR_FIELD(skip_mark_restore);
876876
COPY_NODE_FIELD(mergeclauses);
877877
numCols = list_length(from->mergeclauses);
878-
if (numCols > 0)
879-
{
880-
COPY_POINTER_FIELD(mergeFamilies, numCols * sizeof(Oid));
881-
COPY_POINTER_FIELD(mergeCollations, numCols * sizeof(Oid));
882-
COPY_POINTER_FIELD(mergeStrategies, numCols * sizeof(int));
883-
COPY_POINTER_FIELD(mergeNullsFirst, numCols * sizeof(bool));
884-
}
878+
COPY_POINTER_FIELD(mergeFamilies, numCols * sizeof(Oid));
879+
COPY_POINTER_FIELD(mergeCollations, numCols * sizeof(Oid));
880+
COPY_POINTER_FIELD(mergeStrategies, numCols * sizeof(int));
881+
COPY_POINTER_FIELD(mergeNullsFirst, numCols * sizeof(bool));
885882

886883
return newnode;
887884
}
@@ -1016,12 +1013,9 @@ _copyAgg(const Agg *from)
10161013
COPY_SCALAR_FIELD(aggstrategy);
10171014
COPY_SCALAR_FIELD(aggsplit);
10181015
COPY_SCALAR_FIELD(numCols);
1019-
if (from->numCols > 0)
1020-
{
1021-
COPY_POINTER_FIELD(grpColIdx, from->numCols * sizeof(AttrNumber));
1022-
COPY_POINTER_FIELD(grpOperators, from->numCols * sizeof(Oid));
1023-
COPY_POINTER_FIELD(grpCollations, from->numCols * sizeof(Oid));
1024-
}
1016+
COPY_POINTER_FIELD(grpColIdx, from->numCols * sizeof(AttrNumber));
1017+
COPY_POINTER_FIELD(grpOperators, from->numCols * sizeof(Oid));
1018+
COPY_POINTER_FIELD(grpCollations, from->numCols * sizeof(Oid));
10251019
COPY_SCALAR_FIELD(numGroups);
10261020
COPY_SCALAR_FIELD(transitionSpace);
10271021
COPY_BITMAPSET_FIELD(aggParams);
@@ -1043,19 +1037,13 @@ _copyWindowAgg(const WindowAgg *from)
10431037

10441038
COPY_SCALAR_FIELD(winref);
10451039
COPY_SCALAR_FIELD(partNumCols);
1046-
if (from->partNumCols > 0)
1047-
{
1048-
COPY_POINTER_FIELD(partColIdx, from->partNumCols * sizeof(AttrNumber));
1049-
COPY_POINTER_FIELD(partOperators, from->partNumCols * sizeof(Oid));
1050-
COPY_POINTER_FIELD(partCollations, from->partNumCols * sizeof(Oid));
1051-
}
1040+
COPY_POINTER_FIELD(partColIdx, from->partNumCols * sizeof(AttrNumber));
1041+
COPY_POINTER_FIELD(partOperators, from->partNumCols * sizeof(Oid));
1042+
COPY_POINTER_FIELD(partCollations, from->partNumCols * sizeof(Oid));
10521043
COPY_SCALAR_FIELD(ordNumCols);
1053-
if (from->ordNumCols > 0)
1054-
{
1055-
COPY_POINTER_FIELD(ordColIdx, from->ordNumCols * sizeof(AttrNumber));
1056-
COPY_POINTER_FIELD(ordOperators, from->ordNumCols * sizeof(Oid));
1057-
COPY_POINTER_FIELD(ordCollations, from->ordNumCols * sizeof(Oid));
1058-
}
1044+
COPY_POINTER_FIELD(ordColIdx, from->ordNumCols * sizeof(AttrNumber));
1045+
COPY_POINTER_FIELD(ordOperators, from->ordNumCols * sizeof(Oid));
1046+
COPY_POINTER_FIELD(ordCollations, from->ordNumCols * sizeof(Oid));
10591047
COPY_SCALAR_FIELD(frameOptions);
10601048
COPY_NODE_FIELD(startOffset);
10611049
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
@@ -8886,12 +8886,20 @@ exp_var(const NumericVar *arg, NumericVar *result, int rscale)
88868886
*
88878887
* Essentially, we're approximating log10(abs(ln(var))). This is used to
88888888
* determine the appropriate rscale when computing natural logarithms.
8889+
*
8890+
* Note: many callers call this before range-checking the input. Therefore,
8891+
* we must be robust against values that are invalid to apply ln() to.
8892+
* We don't wish to throw an error here, so just return zero in such cases.
88898893
*/
88908894
static int
88918895
estimate_ln_dweight(const NumericVar *var)
88928896
{
88938897
int ln_dweight;
88948898

8899+
/* Caller should fail on ln(negative), but for the moment return zero */
8900+
if (var->sign != NUMERIC_POS)
8901+
return 0;
8902+
88958903
if (cmp_var(var, &const_zero_point_nine) >= 0 &&
88968904
cmp_var(var, &const_one_point_one) <= 0)
88978905
{

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -594,12 +594,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, VirtualTransactionId *sourcevxid,
594594
CurrentSnapshot->xmax = sourcesnap->xmax;
595595
CurrentSnapshot->xcnt = sourcesnap->xcnt;
596596
Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
597-
memcpy(CurrentSnapshot->xip, sourcesnap->xip,
598-
sourcesnap->xcnt * sizeof(TransactionId));
597+
if (sourcesnap->xcnt > 0)
598+
memcpy(CurrentSnapshot->xip, sourcesnap->xip,
599+
sourcesnap->xcnt * sizeof(TransactionId));
599600
CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
600601
Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
601-
memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
602-
sourcesnap->subxcnt * sizeof(TransactionId));
602+
if (sourcesnap->subxcnt > 0)
603+
memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
604+
sourcesnap->subxcnt * sizeof(TransactionId));
603605
CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
604606
CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
605607
/* 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
@@ -910,7 +910,8 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
910910

911911
more_col_wrapping = col_count;
912912
curr_nl_line = 0;
913-
memset(header_done, false, col_count * sizeof(bool));
913+
if (col_count > 0)
914+
memset(header_done, false, col_count * sizeof(bool));
914915
while (more_col_wrapping)
915916
{
916917
if (opt_border == 2)

0 commit comments

Comments
 (0)