Skip to content

Commit f727b6e

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 72918ea commit f727b6e

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 CLogControlLock, we update the status

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5257,8 +5257,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
52575257
{
52585258
if (FullTransactionIdIsValid(s->fullTransactionId))
52595259
workspace[i++] = XidFromFullTransactionId(s->fullTransactionId);
5260-
memcpy(&workspace[i], s->childXids,
5261-
s->nChildXids * sizeof(TransactionId));
5260+
if (s->nChildXids > 0)
5261+
memcpy(&workspace[i], s->childXids,
5262+
s->nChildXids * sizeof(TransactionId));
52625263
i += s->nChildXids;
52635264
}
52645265
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) */
@@ -293,12 +296,9 @@ _copyRecursiveUnion(const RecursiveUnion *from)
293296
*/
294297
COPY_SCALAR_FIELD(wtParam);
295298
COPY_SCALAR_FIELD(numCols);
296-
if (from->numCols > 0)
297-
{
298-
COPY_POINTER_FIELD(dupColIdx, from->numCols * sizeof(AttrNumber));
299-
COPY_POINTER_FIELD(dupOperators, from->numCols * sizeof(Oid));
300-
COPY_POINTER_FIELD(dupCollations, from->numCols * sizeof(Oid));
301-
}
299+
COPY_POINTER_FIELD(dupColIdx, from->numCols * sizeof(AttrNumber));
300+
COPY_POINTER_FIELD(dupOperators, from->numCols * sizeof(Oid));
301+
COPY_POINTER_FIELD(dupCollations, from->numCols * sizeof(Oid));
302302
COPY_SCALAR_FIELD(numGroups);
303303

304304
return newnode;
@@ -872,13 +872,10 @@ _copyMergeJoin(const MergeJoin *from)
872872
COPY_SCALAR_FIELD(skip_mark_restore);
873873
COPY_NODE_FIELD(mergeclauses);
874874
numCols = list_length(from->mergeclauses);
875-
if (numCols > 0)
876-
{
877-
COPY_POINTER_FIELD(mergeFamilies, numCols * sizeof(Oid));
878-
COPY_POINTER_FIELD(mergeCollations, numCols * sizeof(Oid));
879-
COPY_POINTER_FIELD(mergeStrategies, numCols * sizeof(int));
880-
COPY_POINTER_FIELD(mergeNullsFirst, numCols * sizeof(bool));
881-
}
875+
COPY_POINTER_FIELD(mergeFamilies, numCols * sizeof(Oid));
876+
COPY_POINTER_FIELD(mergeCollations, numCols * sizeof(Oid));
877+
COPY_POINTER_FIELD(mergeStrategies, numCols * sizeof(int));
878+
COPY_POINTER_FIELD(mergeNullsFirst, numCols * sizeof(bool));
882879

883880
return newnode;
884881
}
@@ -979,12 +976,9 @@ _copyAgg(const Agg *from)
979976
COPY_SCALAR_FIELD(aggstrategy);
980977
COPY_SCALAR_FIELD(aggsplit);
981978
COPY_SCALAR_FIELD(numCols);
982-
if (from->numCols > 0)
983-
{
984-
COPY_POINTER_FIELD(grpColIdx, from->numCols * sizeof(AttrNumber));
985-
COPY_POINTER_FIELD(grpOperators, from->numCols * sizeof(Oid));
986-
COPY_POINTER_FIELD(grpCollations, from->numCols * sizeof(Oid));
987-
}
979+
COPY_POINTER_FIELD(grpColIdx, from->numCols * sizeof(AttrNumber));
980+
COPY_POINTER_FIELD(grpOperators, from->numCols * sizeof(Oid));
981+
COPY_POINTER_FIELD(grpCollations, from->numCols * sizeof(Oid));
988982
COPY_SCALAR_FIELD(numGroups);
989983
COPY_BITMAPSET_FIELD(aggParams);
990984
COPY_NODE_FIELD(groupingSets);
@@ -1005,19 +999,13 @@ _copyWindowAgg(const WindowAgg *from)
1005999

10061000
COPY_SCALAR_FIELD(winref);
10071001
COPY_SCALAR_FIELD(partNumCols);
1008-
if (from->partNumCols > 0)
1009-
{
1010-
COPY_POINTER_FIELD(partColIdx, from->partNumCols * sizeof(AttrNumber));
1011-
COPY_POINTER_FIELD(partOperators, from->partNumCols * sizeof(Oid));
1012-
COPY_POINTER_FIELD(partCollations, from->partNumCols * sizeof(Oid));
1013-
}
1002+
COPY_POINTER_FIELD(partColIdx, from->partNumCols * sizeof(AttrNumber));
1003+
COPY_POINTER_FIELD(partOperators, from->partNumCols * sizeof(Oid));
1004+
COPY_POINTER_FIELD(partCollations, from->partNumCols * sizeof(Oid));
10141005
COPY_SCALAR_FIELD(ordNumCols);
1015-
if (from->ordNumCols > 0)
1016-
{
1017-
COPY_POINTER_FIELD(ordColIdx, from->ordNumCols * sizeof(AttrNumber));
1018-
COPY_POINTER_FIELD(ordOperators, from->ordNumCols * sizeof(Oid));
1019-
COPY_POINTER_FIELD(ordCollations, from->ordNumCols * sizeof(Oid));
1020-
}
1006+
COPY_POINTER_FIELD(ordColIdx, from->ordNumCols * sizeof(AttrNumber));
1007+
COPY_POINTER_FIELD(ordOperators, from->ordNumCols * sizeof(Oid));
1008+
COPY_POINTER_FIELD(ordCollations, from->ordNumCols * sizeof(Oid));
10211009
COPY_SCALAR_FIELD(frameOptions);
10221010
COPY_NODE_FIELD(startOffset);
10231011
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
@@ -8130,12 +8130,20 @@ exp_var(const NumericVar *arg, NumericVar *result, int rscale)
81308130
*
81318131
* Essentially, we're approximating log10(abs(ln(var))). This is used to
81328132
* determine the appropriate rscale when computing natural logarithms.
8133+
*
8134+
* Note: many callers call this before range-checking the input. Therefore,
8135+
* we must be robust against values that are invalid to apply ln() to.
8136+
* We don't wish to throw an error here, so just return zero in such cases.
81338137
*/
81348138
static int
81358139
estimate_ln_dweight(const NumericVar *var)
81368140
{
81378141
int ln_dweight;
81388142

8143+
/* Caller should fail on ln(negative), but for the moment return zero */
8144+
if (var->sign != NUMERIC_POS)
8145+
return 0;
8146+
81398147
if (cmp_var(var, &const_zero_point_nine) >= 0 &&
81408148
cmp_var(var, &const_one_point_one) <= 0)
81418149
{

‎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
@@ -913,7 +913,8 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
913913

914914
more_col_wrapping = col_count;
915915
curr_nl_line = 0;
916-
memset(header_done, false, col_count * sizeof(bool));
916+
if (col_count > 0)
917+
memset(header_done, false, col_count * sizeof(bool));
917918
while (more_col_wrapping)
918919
{
919920
if (opt_border == 2)

0 commit comments

Comments
 (0)