Thread: [BUG] (firsttupleslot)==NULL is redundant or is possible nulldereference?

[BUG] (firsttupleslot)==NULL is redundant or is possible nulldereference?

From

Ranier Vilela

Date:

23 November 2019, 01:32:11

Hi,
This is real bug? firsttupleslot == NULL.

\backend\executor\nodeGroup.c
    if (TupIsNull(firsttupleslot))
    {
        outerslot = ExecProcNode(outerPlanState(node));
        if (TupIsNull(outerslot))
        {
            /* empty input, so return nothing */
            node->grp_done = true;
            return NULL;
        }
        /* Copy tuple into firsttupleslot */
        ExecCopySlot(firsttupleslot, outerslot);

include\executor\tuptable.h:
#define TupIsNull(slot) \
    ((slot) == NULL || TTS_EMPTY(slot))

static inline TupleTableSlot *
ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
{
    Assert(!TTS_EMPTY(srcslot));

    dstslot->tts_ops->copyslot(dstslot, srcslot);

    return dstslot;
}

Re: [BUG] (firsttupleslot)==NULL is redundant or is possible nulldereference?

From

Tomas Vondra

Date:

23 November 2019, 01:54:15

On Fri, Nov 22, 2019 at 10:32:11PM +0000, Ranier Vilela wrote:
>Hi,
>This is real bug? firsttupleslot == NULL.
>

Ranier, I don't want to be rude, but I personally am getting a bit
annoyed by this torrent of bug reports that are essentially just a bunch
of copy-pasted chunks of code, without any specification of bench,
position in the file, etc.

And more importantly, without any clear explanation why you think it is
a bug (or even a demonstration of an issue), and "Is this a bug?"

>\backend\executor\nodeGroup.c
>    if (TupIsNull(firsttupleslot))
>    {
>        outerslot = ExecProcNode(outerPlanState(node));
>        if (TupIsNull(outerslot))
>        {
>            /* empty input, so return nothing */
>            node->grp_done = true;
>            return NULL;
>        }
>        /* Copy tuple into firsttupleslot */
>        ExecCopySlot(firsttupleslot, outerslot);
>
>include\executor\tuptable.h:
>#define TupIsNull(slot) \
>    ((slot) == NULL || TTS_EMPTY(slot))
>
>static inline TupleTableSlot *
>ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
>{
>    Assert(!TTS_EMPTY(srcslot));
>
>    dstslot->tts_ops->copyslot(dstslot, srcslot);
>
>    return dstslot;
>}
>

And why do you think this is a bug? Immediately before the part of code
you copied we have this:

     /*
      * The ScanTupleSlot holds the (copied) first tuple of each group.
      */
     firsttupleslot = node->ss.ss_ScanTupleSlot;

And node->ss.ss_ScanTupleSlot is expected to be non-NULL. So the initial
assumption that firsttupleslot is NULL is incorrect.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

RE: [BUG] (firsttupleslot)==NULL is redundant or is possible nulldereference?

From

Ranier Vilela

Date:

23 November 2019, 01:57:13

Hi,
Sorry, you are right.
Had not seen this line:
firsttupleslot = node->ss.ss_ScanTupleSlot;

Best regards.
Ranier Vilela
________________________________________
De: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Enviado: sexta-feira, 22 de novembro de 2019 22:54
Para: Ranier Vilela
Cc: pgsql-hackers@postgresql.org
Assunto: Re: [BUG] (firsttupleslot)==NULL is redundant or is possible null dereference?

On Fri, Nov 22, 2019 at 10:32:11PM +0000, Ranier Vilela wrote:
>Hi,
>This is real bug? firsttupleslot == NULL.
>

Ranier, I don't want to be rude, but I personally am getting a bit
annoyed by this torrent of bug reports that are essentially just a bunch
of copy-pasted chunks of code, without any specification of bench,
position in the file, etc.

And more importantly, without any clear explanation why you think it is
a bug (or even a demonstration of an issue), and "Is this a bug?"

>\backend\executor\nodeGroup.c
>       if (TupIsNull(firsttupleslot))
>       {
>               outerslot = ExecProcNode(outerPlanState(node));
>               if (TupIsNull(outerslot))
>               {
>                       /* empty input, so return nothing */
>                       node->grp_done = true;
>                       return NULL;
>               }
>               /* Copy tuple into firsttupleslot */
>               ExecCopySlot(firsttupleslot, outerslot);
>
>include\executor\tuptable.h:
>#define TupIsNull(slot) \
>       ((slot) == NULL || TTS_EMPTY(slot))
>
>static inline TupleTableSlot *
>ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
>{
>       Assert(!TTS_EMPTY(srcslot));
>
>       dstslot->tts_ops->copyslot(dstslot, srcslot);
>
>       return dstslot;
>}
>

And why do you think this is a bug? Immediately before the part of code
you copied we have this:

     /*
      * The ScanTupleSlot holds the (copied) first tuple of each group.
      */
     firsttupleslot = node->ss.ss_ScanTupleSlot;

And node->ss.ss_ScanTupleSlot is expected to be non-NULL. So the initial
assumption that firsttupleslot is NULL is incorrect.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUG] (firsttupleslot)==NULL is redundant or is possible nulldereference?

From

Tomas Vondra

Date:

23 November 2019, 02:12:48

On Fri, Nov 22, 2019 at 10:57:13PM +0000, Ranier Vilela wrote:
>Hi,
>Sorry, you are right.
>Had not seen this line:
>firsttupleslot = node->ss.ss_ScanTupleSlot;
>

OK, no problem. When writing future messages to this list, please

* Make sure you explain why you think a given code is broken. Ideally,
   bug reports come with a reproducer (instructions how to hit it) but
   that may be difficult in some cases.

* Don't top post, but respond in-line. Top posting makes it much harder
   to follow the discussion, in-line replies are customary here.

* Don't mark questions as bugs in the subject.

Otherwise you'll just annoy people to the extent that they'll start
ignoring your posts entirely.

We're OK with answering querstions and helping people learn the code
base, but the other side needs to make a bit of effort too.

regards

>Best regards.
>Ranier Vilela
>________________________________________
>De: Tomas Vondra <tomas.vondra@2ndquadrant.com>
>Enviado: sexta-feira, 22 de novembro de 2019 22:54
>Para: Ranier Vilela
>Cc: pgsql-hackers@postgresql.org
>Assunto: Re: [BUG] (firsttupleslot)==NULL is redundant or is possible null dereference?
>
>On Fri, Nov 22, 2019 at 10:32:11PM +0000, Ranier Vilela wrote:
>>Hi,
>>This is real bug? firsttupleslot == NULL.
>>
>
>Ranier, I don't want to be rude, but I personally am getting a bit
>annoyed by this torrent of bug reports that are essentially just a bunch
>of copy-pasted chunks of code, without any specification of bench,
>position in the file, etc.
>
>And more importantly, without any clear explanation why you think it is
>a bug (or even a demonstration of an issue), and "Is this a bug?"
>
>>\backend\executor\nodeGroup.c
>>       if (TupIsNull(firsttupleslot))
>>       {
>>               outerslot = ExecProcNode(outerPlanState(node));
>>               if (TupIsNull(outerslot))
>>               {
>>                       /* empty input, so return nothing */
>>                       node->grp_done = true;
>>                       return NULL;
>>               }
>>               /* Copy tuple into firsttupleslot */
>>               ExecCopySlot(firsttupleslot, outerslot);
>>
>>include\executor\tuptable.h:
>>#define TupIsNull(slot) \
>>       ((slot) == NULL || TTS_EMPTY(slot))
>>
>>static inline TupleTableSlot *
>>ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
>>{
>>       Assert(!TTS_EMPTY(srcslot));
>>
>>       dstslot->tts_ops->copyslot(dstslot, srcslot);
>>
>>       return dstslot;
>>}
>>
>
>And why do you think this is a bug? Immediately before the part of code
>you copied we have this:
>
>     /*
>      * The ScanTupleSlot holds the (copied) first tuple of each group.
>      */
>     firsttupleslot = node->ss.ss_ScanTupleSlot;
>
>And node->ss.ss_ScanTupleSlot is expected to be non-NULL. So the initial
>assumption that firsttupleslot is NULL is incorrect.
>
>regards
>
>--
>Tomas Vondra                  http://www.2ndQuadrant.com
>PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

RE: [BUG] (firsttupleslot)==NULL is redundant or is possible nulldereference?

From

Ranier Vilela

Date:

23 November 2019, 06:38:23

>And why do you think this is a bug? Immediately before the part of code
>you copied we have this:
>
>     /*
>      * The ScanTupleSlot holds the (copied) first tuple of each group.
>      */
>     firsttupleslot = node->ss.ss_ScanTupleSlot;
>And node->ss.ss_ScanTupleSlot is expected to be non-NULL. So the initial
>assumption that firsttupleslot is NULL is incorrect.

IMHO, the test could be improved, this way it silences the scan tool.

--- \dll\postgresql-12.0\a\backend\executor\nodeGroup.c    Mon Sep 30 17:06:55 2019
+++ nodeGroup.c    Sat Nov 23 00:23:27 2019
@@ -64,7 +64,7 @@
      * If first time through, acquire first input tuple and determine whether
      * to return it or not.
      */
-    if (TupIsNull(firsttupleslot))
+    if ((firsttupleslot != NULL) && TTS_EMPTY(firsttupleslot))
     {
         outerslot = ExecProcNode(outerPlanState(node));
         if (TupIsNull(outerslot))

best regards.
Ranier Vilela