Fix NULL pointer reference in _outPathTarget()

Richard Guo <guofenglinux@gmail.com> 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

On Tue, Apr 19, 2022 at 2:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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

On 2022-Apr-18, Tom Lane wrote:

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.

Yeah -- looking at the script to generate node support functions[1]/messages/by-id/bee9fdb0-cd10-5fdb-3027-c4b5a240bc74@enterprisedb.com, 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]: /messages/by-id/bee9fdb0-cd10-5fdb-3027-c4b5a240bc74@enterprisedb.com

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

On 18.04.22 09:35, Richard Guo wrote:

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.

Do you have a test case that triggers this issue?

On 18.04.22 20:53, Tom Lane wrote:

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.

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

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 18.04.22 20:53, Tom Lane wrote:

A semantics-preserving conversion would have looked something like
if (node->sortgrouprefs)
WRITE_INDEX_ARRAY(sortgrouprefs, list_length(node->exprs));

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

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

On Thu, Apr 21, 2022 at 12:02 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

On 18.04.22 09:35, Richard Guo wrote:

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.

Do you have a test case that triggers this issue?

I don't have a test case. :( I triggered this issue while debugging
with gdb and I was printing a certain 'pathlist' with nodeToString().

If it helps, here is the backtrace:

#0 in _outPathTarget (str=0x7fff683d7e50, node=0x56011e5cece0) at
outfuncs.c:2672
#1 in outNode (str=0x7fff683d7e50, obj=0x56011e5cece0) at outfuncs.c:4490
#2 in _outPathInfo (str=0x7fff683d7e50, node=0x56011e5f3408) at
outfuncs.c:1922
#3 in _outPath (str=0x7fff683d7e50, node=0x56011e5f3408) at outfuncs.c:1957
#4 in outNode (str=0x7fff683d7e50, obj=0x56011e5f3408) at outfuncs.c:4358
#5 in _outProjectionPath (str=0x7fff683d7e50, node=0x56011e5f3890) at
outfuncs.c:2154
#6 in outNode (str=0x7fff683d7e50, obj=0x56011e5f3890) at outfuncs.c:4409
#7 in _outAggPath (str=0x7fff683d7e50, node=0x56011e5f4550) at
outfuncs.c:2224
#8 in outNode (str=0x7fff683d7e50, obj=0x56011e5f4550) at outfuncs.c:4427
#9 in _outGatherPath (str=0x7fff683d7e50, node=0x56011e5f45e8) at
outfuncs.c:2142
#10 in outNode (str=0x7fff683d7e50, obj=0x56011e5f45e8) at outfuncs.c:4406
#11 in _outList (str=0x7fff683d7e50, node=0x56011e5f4680) at outfuncs.c:227
#12 in outNode (str=0x7fff683d7e50, obj=0x56011e5f4680) at outfuncs.c:4028
#13 in nodeToString (obj=0x56011e5f4680) at outfuncs.c:4782

Thanks
Richard

On 20.04.22 18:53, Tom Lane wrote:

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

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.

We could adapt the convention to print NULL values as "<>", like

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 6a02f81ad5..4eb5be3787 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -127,8 +127,11 @@ static void outChar(StringInfo str, char c);
  #define WRITE_INDEX_ARRAY(fldname, len) \
     do { \
         appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       for (int i = 0; i < len; i++) \
-           appendStringInfo(str, " %u", node->fldname[i]); \
+       if (node->fldname) \
+           for (int i = 0; i < len; i++) \
+               appendStringInfo(str, " %u", node->fldname[i]); \
+       else \
+           appendStringInfoString(str, "<>"); \
     } while(0)

#define WRITE_INT_ARRAY(fldname, len) \

There is currently no read function for this that would need to be
changed. But looking at peers such as WRITE_INT_ARRAY/READ_INT_ARRAY it
shouldn't be hard to sort out if it became necessary.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 20.04.22 18:53, Tom Lane wrote:

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.

We could adapt the convention to print NULL values as "<>", like

Works for me.

regards, tom lane

On 22.04.22 16:18, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 20.04.22 18:53, Tom Lane wrote:

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.

We could adapt the convention to print NULL values as "<>", like

Works for me.

done