Re: Undefined behavior detected by new clang's ubsan

On Wed, Jan 21, 2026 at 1:52 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> John Naylor <johncnaylorls(at)gmail(dot)com> 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)