On Wed, Jan 21, 2026 at 1:52 AM Tom Lane <[email protected]> wrote:
>
> John Naylor <[email protected]> writes:
> > I don't think it's great to pass a NULL pointer to a sort, but the
> > length could conceivably be zero for future degenerate cases, so we
> > could silence the warning by adding "if (n < 2) return;" before the
> > for-loop. The advantage of doing that anyway is it allows us to remove
> > all four of the "if (d_ > ST_POINTER_STEP)" branches in the recursion
> > part. That's better for readability.
>
> +1
Okay, I've written that approach. Since it requires a bit more
explanation, I've kept it separate for now.
> With the attached patch applied, `make check-world` passes for me.
As for the rest of the proposed fixes, most seem okay, but I have some nits:
trgm_gist.c:
- TRGM *cachedVal = (TRGM *) (cache + MAXALIGN(siglen));
+ TRGM *cachedVal = cache ? ((TRGM *) (cache + MAXALIGN(siglen))) : NULL;
This is getting a bit unwieldy for a declaration. How about this?
char *cache = (char *) fcinfo->flinfo->fn_extra;
- TRGM *cachedVal = (TRGM *) (cache + MAXALIGN(siglen));
+ TRGM *cachedVal = NULL;
[...]
+ if (cache != NULL)
+ cachedVal = (TRGM *) (cache + MAXALIGN(siglen));
heaptoast.c
memcpy(VARDATA(result) +
- (curchunk * TOAST_MAX_CHUNK_SIZE - sliceoffset) + chcpystrt,
+ (int)(curchunk * TOAST_MAX_CHUNK_SIZE - sliceoffset) + chcpystrt,
Not sure about this one. It would be better if we reversing the
operands allowed us to avoid overflow in the first place:
- (curchunk * TOAST_MAX_CHUNK_SIZE - sliceoffset) + chcpystrt,
+ chcpystrt + (curchunk * TOAST_MAX_CHUNK_SIZE - sliceoffset)
Does that silence the warning?
sharedtuplestore.c
- if (accessor->write_pointer + size > accessor->write_end)
+ if ((accessor->write_pointer == NULL && accessor->write_end == NULL
&& size > 0) || (accessor->write_pointer + size >
accessor->write_end))
{
if (accessor->write_chunk == NULL)
{
/* First time through. Allocate chunk. */
I don't see why we have to check so many conditions. The last line
above is where write_pointer is set in a new allocation, so it seems
we could just have
if (accessor->write_pointer == NULL ||
accessor->write_pointer + size > accessor->write_end)
From dbf23d519d6a11d3ecc1220eb38d67ece5ae52d8 Mon Sep 17 00:00:00 2001
From: John Naylor <[email protected]>
Date: Wed, 21 Jan 2026 16:44:46 +0700
Subject: [PATCH v1 2/2] Fix various cases of undefined behavior
Reported-by: Alexander Lakhin <[email protected]>
Tested-by:
Discussion: https://postgr.es/m/[email protected]
---
contrib/pg_trgm/trgm_gist.c | 5 ++++-
src/backend/access/heap/heaptoast.c | 2 +-
src/backend/utils/sort/sharedtuplestore.c | 3 ++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_trgm/trgm_gist.c b/contrib/pg_trgm/trgm_gist.c
index 5c7deb103a6..b6ad13cc0a4 100644
--- a/contrib/pg_trgm/trgm_gist.c
+++ b/contrib/pg_trgm/trgm_gist.c
@@ -699,10 +699,13 @@ gtrgm_penalty(PG_FUNCTION_ARGS)
if (ISARRKEY(newval))
{
char *cache = (char *) fcinfo->flinfo->fn_extra;
- TRGM *cachedVal = (TRGM *) (cache + MAXALIGN(siglen));
+ TRGM *cachedVal = NULL;
Size newvalsize = VARSIZE(newval);
BITVECP sign;
+ if (cache != NULL)
+ cachedVal = (TRGM *) (cache + MAXALIGN(siglen));
+
/*
* Cache the sign data across multiple calls with the same newval.
*/
diff --git a/src/backend/access/heap/heaptoast.c b/src/backend/access/heap/heaptoast.c
index e28fe47a449..3fb4a8d2a49 100644
--- a/src/backend/access/heap/heaptoast.c
+++ b/src/backend/access/heap/heaptoast.c
@@ -768,7 +768,7 @@ heap_fetch_toast_slice(Relation toastrel, Oid valueid, int32 attrsize,
chcpyend = (sliceoffset + slicelength - 1) % TOAST_MAX_CHUNK_SIZE;
memcpy(VARDATA(result) +
- (curchunk * TOAST_MAX_CHUNK_SIZE - sliceoffset) + chcpystrt,
+ chcpystrt + (curchunk * TOAST_MAX_CHUNK_SIZE - sliceoffset),
chunkdata + chcpystrt,
(chcpyend - chcpystrt) + 1);
diff --git a/src/backend/utils/sort/sharedtuplestore.c b/src/backend/utils/sort/sharedtuplestore.c
index 8f35a255263..04189f708fa 100644
--- a/src/backend/utils/sort/sharedtuplestore.c
+++ b/src/backend/utils/sort/sharedtuplestore.c
@@ -323,7 +323,8 @@ sts_puttuple(SharedTuplestoreAccessor *accessor, void *meta_data,
/* Do we have space? */
size = accessor->sts->meta_data_size + tuple->t_len;
- if (accessor->write_pointer + size > accessor->write_end)
+ if (accessor->write_pointer == NULL ||
+ accessor->write_pointer + size > accessor->write_end)
{
if (accessor->write_chunk == NULL)
{
--
2.52.0
From d0695c513d88a51e8fd71a2d59b05dfffc4c8590 Mon Sep 17 00:00:00 2001
From: John Naylor <[email protected]>
Date: Wed, 21 Jan 2026 15:21:31 +0700
Subject: [PATCH v1 1/2] Silence clang ubsan warning in sort template
make_multirange() is sometimes called with NULL ranges and zero
count. This leads to a qsort with a NULL pointer for the input
array. The sort template short-cicuits during insertion sort with
zero count, so this doesn't cause a crash, but the calculation sets
off a warning in clang 21 sanitizers. We can silence the warning by
checking that there are at least two elements to sort, and only then
perform arithmetic on the input pointer. Since we now check the length
at the start of the function, also remove the analagous checks that
guarded against recursion/iteration.
Reviewed-by:
Reported-by: Alexander Lakhin <[email protected]>
Tested-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch-through:
---
src/include/lib/sort_template.h | 40 +++++++++++++++++----------------
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/src/include/lib/sort_template.h b/src/include/lib/sort_template.h
index e02aa73cd4d..22b2092d03b 100644
--- a/src/include/lib/sort_template.h
+++ b/src/include/lib/sort_template.h
@@ -311,6 +311,14 @@ loop:
DO_CHECK_FOR_INTERRUPTS();
if (n < 7)
{
+ /*
+ * Not strictly necessary, but a caller may pass a NULL pointer input
+ * and zero length, and this silences warnings about applying offsets
+ * to NULL pointers.
+ */
+ if (n < 2)
+ return;
+
for (pm = a + ST_POINTER_STEP; pm < a + n * ST_POINTER_STEP;
pm += ST_POINTER_STEP)
for (pl = pm; pl > a && DO_COMPARE(pl - ST_POINTER_STEP, pl) > 0;
@@ -387,29 +395,23 @@ loop:
if (d1 <= d2)
{
/* Recurse on left partition, then iterate on right partition */
- if (d1 > ST_POINTER_STEP)
- DO_SORT(a, d1 / ST_POINTER_STEP);
- if (d2 > ST_POINTER_STEP)
- {
- /* Iterate rather than recurse to save stack space */
- /* DO_SORT(pn - d2, d2 / ST_POINTER_STEP) */
- a = pn - d2;
- n = d2 / ST_POINTER_STEP;
- goto loop;
- }
+ DO_SORT(a, d1 / ST_POINTER_STEP);
+
+ /* Iterate rather than recurse to save stack space */
+ /* DO_SORT(pn - d2, d2 / ST_POINTER_STEP) */
+ a = pn - d2;
+ n = d2 / ST_POINTER_STEP;
+ goto loop;
}
else
{
/* Recurse on right partition, then iterate on left partition */
- if (d2 > ST_POINTER_STEP)
- DO_SORT(pn - d2, d2 / ST_POINTER_STEP);
- if (d1 > ST_POINTER_STEP)
- {
- /* Iterate rather than recurse to save stack space */
- /* DO_SORT(a, d1 / ST_POINTER_STEP) */
- n = d1 / ST_POINTER_STEP;
- goto loop;
- }
+ DO_SORT(pn - d2, d2 / ST_POINTER_STEP);
+
+ /* Iterate rather than recurse to save stack space */
+ /* DO_SORT(a, d1 / ST_POINTER_STEP) */
+ n = d1 / ST_POINTER_STEP;
+ goto loop;
}
}
#endif
--
2.52.0
