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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Ranier Vilela
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;
}


Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
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


Reply | Threaded
Open this post in threaded view
|

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

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

Best regards.
Ranier Vilela
________________________________________
De: Tomas Vondra <[hidden email]>
Enviado: sexta-feira, 22 de novembro de 2019 22:54
Para: Ranier Vilela
Cc: [hidden email]
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


Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
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 <[hidden email]>
>Enviado: sexta-feira, 22 de novembro de 2019 22:54
>Para: Ranier Vilela
>Cc: [hidden email]
>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


Reply | Threaded
Open this post in threaded view
|

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

Ranier Vilela
In reply to this post by Tomas Vondra-4
>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