Subject: Re: Fix NULL pointer reference in _outPathTarget()

From: Tom Lane <[email-masked]>
Date: Mon, 18 Apr 2022 14:53:41 -0400

Richard Guo <[email-masked]> writes:
> The array sortgrouprefs[] inside PathTarget might be NULL if we have not
> identified sort/group columns in this tlist. In that case we would have
> a NULL pointer reference in _outPathTarget() when trying to print
> sortgrouprefs[] with WRITE_INDEX_ARRAY as we are using the length of
> PathTarget->exprs as its array length.

I wondered why we'd not noticed this long since, and the answer is that
it got broken relatively recently by bdeb2c4ec, which removed the former
conditionality of the code:

@@ -2510,14 +2517,7 @@ _outPathTarget(StringInfo str, const PathTarget *node)
     WRITE_NODE_TYPE("PATHTARGET");
 
     WRITE_NODE_FIELD(exprs);
-    if (node->sortgrouprefs)
-    {
-        int            i;
-
-        appendStringInfoString(str, " :sortgrouprefs");
-        for (i = 0; i < list_length(node->exprs); i++)
-            appendStringInfo(str, " %u", node->sortgrouprefs[i]);
-    }
+    WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));
     WRITE_FLOAT_FIELD(cost.startup, "%.2f");
     WRITE_FLOAT_FIELD(cost.per_tuple, "%.2f");
     WRITE_INT_FIELD(width);

A semantics-preserving conversion would have looked something like

    if (node->sortgrouprefs)
        WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));

I suppose that Peter was trying to remove special cases from the
outfuncs.c code, but do we want to put this one back?  Richard's
proposal would not accurately reflect the contents of the data
structure, so I'm not too thrilled with it.

			regards, tom lane

---

From: Richard Guo <[email-masked]>
Date: Tue, 19 Apr 2022 10:51:49 +0800

> structure, so I'm not too thrilled with it.

The commit message in bdeb2c4ec mentions that:

"
This also changes the behavior slightly: Before, the field name was
skipped if the length was zero.  Now it prints the field name even in
that case.  This is more consistent with how other array fields are
handled.
"

So I suppose we are trying to print the field name even if the length is
zero. Should we keep this behavior in the fix?

Thanks
Richard

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 19 Apr 2022 11:53:33 +0200

> structure, so I'm not too thrilled with it.

Yeah -- looking at the script to generate node support functions[1], it
might be better go back to the original formulation (i.e., your proposed
patch), and then use a "path_hack4" for this struct member, which looks
similar to other hacks already there for other cases that require
bespoke handling.

[1] https://postgr.es/m/[email-masked]

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 20 Apr 2022 18:04:00 +0200

> structure, so I'm not too thrilled with it.

I think we could put the if (node->fldname) inside the WRITE_INDEX_ARRAY 
macro.

---

From: Tom Lane <[email-masked]>
Date: Wed, 20 Apr 2022 12:53:50 -0400

> I think we could put the if (node->fldname) inside the WRITE_INDEX_ARRAY 

Yeah, that's another way to do it.  I think though that the unresolved
question is whether or not we want the field name to appear in the output
when the field is null.  I believe that I intentionally made it not appear
originally, so that that case could readily be distinguished.  You could
argue that that would complicate life greatly for a _readPathTarget()
function, which is true, but I don't foresee that we'll need one.

			regards, tom lane