Re: Undefined behavior detected by new clang's ubsan

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