Thread: 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 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], 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/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 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