Обсуждение: Explicit NULL dereference (src/backend/utils/adt/ruleutils.c)

At Sat, 31 Oct 2020 11:49:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> Per Coverity.
> 
> make_ruledef function can dereference a NULL pointer (actions),
> if "ev_qual" is provided and "actions" does not exist.
> 
> The comment there is contradictory: " /* these could be nulls */ "
> Because if "ev_qual" is not null, "actions" cannot be either.
> 
> Solution proposed merely as a learning experience.

We cannot reach there with ev_action == NULL since it comes from a
non-nullable column. Since most of the other columns has an assertion
that !isnull, I think we should do the same thing for ev_action (and
ev_qual).  SPI_getvalue() returns C-NULL for SQL-NULL (or for some
other unexpected situations.).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6c656586e8..6ba2b4a5ae 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -4766,12 +4766,13 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
     /* these could be nulls */
     fno = SPI_fnumber(rulettc, "ev_qual");
     ev_qual = SPI_getvalue(ruletup, rulettc, fno);
+    Assert(ev_qual != NULL);
 
     fno = SPI_fnumber(rulettc, "ev_action");
     ev_action = SPI_getvalue(ruletup, rulettc, fno);
-    if (ev_action != NULL)
-        actions = (List *) stringToNode(ev_action);
-
+    Assert(ev_action != NULL);
+    actions = (List *) stringToNode(ev_action);
+        
     ev_relation = table_open(ev_class, AccessShareLock);
 
     /*

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> We cannot reach there with ev_action == NULL since it comes from a
> non-nullable column. Since most of the other columns has an assertion
> that !isnull, I think we should do the same thing for ev_action (and
> ev_qual).  SPI_getvalue() returns C-NULL for SQL-NULL (or for some
> other unexpected situations.).

Isn't the comment just above there wrong?

     /* these could be nulls */

I wonder just when that became outdated.

            regards, tom lane

At Mon, 02 Nov 2020 10:36:10 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Sat, 31 Oct 2020 11:49:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> > Per Coverity.
> > 
> > make_ruledef function can dereference a NULL pointer (actions),
> > if "ev_qual" is provided and "actions" does not exist.
> > 
> > The comment there is contradictory: " /* these could be nulls */ "
> > Because if "ev_qual" is not null, "actions" cannot be either.
> > 
> > Solution proposed merely as a learning experience.
> 
> We cannot reach there with ev_action == NULL since it comes from a
> non-nullable column. Since most of the other columns has an assertion
> that !isnull, I think we should do the same thing for ev_action (and
> ev_qual).  SPI_getvalue() returns C-NULL for SQL-NULL (or for some
> other unexpected situations.).

The following code is found there, since 1998. (15cb32d93e)

>    /* If the rule has an event qualification, add it */
>    if (ev_qual == NULL)
>        ev_qual = "";

The problem code here was written as the follows.

+    fno = SPI_fnumber(rulettc, "is_instead");
+    is_instead = (bool)SPI_getbinval(ruletup, rulettc, fno, &isnull);
+
+    fno = SPI_fnumber(rulettc, "ev_qual");
+    ev_qual = SPI_getvalue(ruletup, rulettc, fno);
+    if (isnull) ev_qual = NULL;
+
+    fno = SPI_fnumber(rulettc, "ev_action");
+    ev_action = SPI_getvalue(ruletup, rulettc, fno);
+    if (isnull) ev_action = NULL;
+    if (ev_action != NULL) {
+        actions = (List *)stringToNode(ev_action);
+    }

I'm not sure what the code means by just reading there but at least it
seems impossible for the current code to return NULL for legit values.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6c656586e8..ab4f102e9b 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -4766,12 +4766,13 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
     /* these could be nulls */
     fno = SPI_fnumber(rulettc, "ev_qual");
     ev_qual = SPI_getvalue(ruletup, rulettc, fno);
+    Assert(ev_qual != NULL);
 
     fno = SPI_fnumber(rulettc, "ev_action");
     ev_action = SPI_getvalue(ruletup, rulettc, fno);
-    if (ev_action != NULL)
-        actions = (List *) stringToNode(ev_action);
-
+    Assert(ev_action != NULL);
+    actions = (List *) stringToNode(ev_action);
+        
     ev_relation = table_open(ev_class, AccessShareLock);
 
     /*
@@ -4819,9 +4820,6 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
                      generate_relation_name(ev_class, NIL) :
                      generate_qualified_relation_name(ev_class));
 
-    /* If the rule has an event qualification, add it */
-    if (ev_qual == NULL)
-        ev_qual = "";
     if (strlen(ev_qual) > 0 && strcmp(ev_qual, "<>") != 0)
     {
         Node       *qual;

At Sun, 01 Nov 2020 21:05:29 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > We cannot reach there with ev_action == NULL since it comes from a
> > non-nullable column. Since most of the other columns has an assertion
> > that !isnull, I think we should do the same thing for ev_action (and
> > ev_qual).  SPI_getvalue() returns C-NULL for SQL-NULL (or for some
> > other unexpected situations.).
> 
> Isn't the comment just above there wrong?
> 
>      /* these could be nulls */
> 
> I wonder just when that became outdated.

Mmm. I investigated that.

At the very beginning of CREATE RULE (d31084e9d1, 1996), InsertRule()
did the following.

>    template = "INSERT INTO pg_rewrite \
>(rulename, ev_type, ev_class, ev_attr, action, ev_qual, is_instead) VALUES \
>('%s', %d::char, %d::oid, %d::int2, '%s'::text, '%s'::text, \
> '%s'::bool);";
>    if (strlen(template) + strlen(rulname) + strlen(actionbuf) +
>    strlen(qualbuf) + 20 /* fudge fac */ >  RULE_PLAN_SIZE) {
>    elog(WARN, "DefineQueryRewrite: rule plan string too big.");
>    }
>    sprintf(rulebuf, template,
>        rulname, evtype, eventrel_oid, evslot_index, actionbuf,
>        qualbuf, is_instead);

Doesn't seem that ev_qual and ev_action can be NULL.  The same
function in the current converts action list to string using
nodeToSTring so NIL is converted into '<>', which is not NULL.

So I think ev_action cannot be null from the beginning of the history
unless the columns is modified manually.  ev_qual and ev_action are
marked as non-nullable (9b39b799db, in 2018). They could be null if we
modified that columns nullable then set NULL, but that could happen on
all other columns in pg_rewite catalog, which are Assert(!null)ed.

Although ev_action cannot be a empty list using SQL interface. So we
can get rid of the case list_length(action) == 0, but I'm not sure
it's worth doing (but the attaches does..).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6c656586e8..120b8869ea 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -4763,15 +4763,15 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
     Assert(!isnull);
     is_instead = DatumGetBool(dat);
 
-    /* these could be nulls */
     fno = SPI_fnumber(rulettc, "ev_qual");
     ev_qual = SPI_getvalue(ruletup, rulettc, fno);
+    Assert(ev_qual != NULL);
 
     fno = SPI_fnumber(rulettc, "ev_action");
     ev_action = SPI_getvalue(ruletup, rulettc, fno);
-    if (ev_action != NULL)
-        actions = (List *) stringToNode(ev_action);
-
+    Assert(ev_action != NULL);
+    actions = (List *) stringToNode(ev_action);
+        
     ev_relation = table_open(ev_class, AccessShareLock);
 
     /*
@@ -4819,10 +4819,8 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
                      generate_relation_name(ev_class, NIL) :
                      generate_qualified_relation_name(ev_class));
 
-    /* If the rule has an event qualification, add it */
-    if (ev_qual == NULL)
-        ev_qual = "";
-    if (strlen(ev_qual) > 0 && strcmp(ev_qual, "<>") != 0)
+    Assert(strlen(ev_qual) > 0);
+    if (strcmp(ev_qual, "<>") != 0)
     {
         Node       *qual;
         Query       *query;
@@ -4875,6 +4873,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
         appendStringInfoString(buf, "INSTEAD ");
 
     /* Finally the rules actions */
+    Assert(list_length(actions) > 0);
     if (list_length(actions) > 1)
     {
         ListCell   *action;
@@ -4893,10 +4892,6 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
         }
         appendStringInfoString(buf, ");");
     }
-    else if (list_length(actions) == 0)
-    {
-        appendStringInfoString(buf, "NOTHING;");
-    }
     else
     {
         Query       *query;

Ranier Vilela <ranier.vf@gmail.com> writes:
> Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi <
> horikyota.ntt@gmail.com> escreveu:
>> Doesn't seem that ev_qual and ev_action can be NULL.  The same
>> function in the current converts action list to string using
>> nodeToSTring so NIL is converted into '<>', which is not NULL.

> I think that Assert is not the right solution here.

I think there's some confusion here: whether the ev_actions column can
contain a SQL NULL is a very different thing from whether the result of
stringToNode() on it can be a NIL list.  The latter should also not
happen, but it's not enforced by low-level code in the same way that
the NOT NULL property is.  So in my judgment, it's okay for us to just
Assert that we got a not-null datum, but it's probably worth expending
an actual test-and-elog for the NIL-list case.

Of course, someone could put a string into that column that doesn't
read out as a List at all, or it does but the elements aren't Query
nodes, etc etc.  There's a very finite limit to how much code I'm
willing to expend on such scenarios.  But a test for NIL list seems
reasonable, since this function previously had what looked like sane
handling for that case.

Anyway, I adjusted Kyotaro-san's patch a bit (including fixing the
near identical code in make_viewdef) and pushed it.

            regards, tom lane

At Mon, 02 Nov 2020 14:49:50 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> Ranier Vilela <ranier.vf@gmail.com> writes:
> > Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi <
> > horikyota.ntt@gmail.com> escreveu:
> >> Doesn't seem that ev_qual and ev_action can be NULL.  The same
> >> function in the current converts action list to string using
> >> nodeToSTring so NIL is converted into '<>', which is not NULL.
>
> > I think that Assert is not the right solution here.
>
> I think there's some confusion here: whether the ev_actions column can
> contain a SQL NULL is a very different thing from whether the result of
> stringToNode() on it can be a NIL list.  The latter should also not
> happen, but it's not enforced by low-level code in the same way that
> the NOT NULL property is.  So in my judgment, it's okay for us to just
> Assert that we got a not-null datum, but it's probably worth expending
> an actual test-and-elog for the NIL-list case.
>
> Of course, someone could put a string into that column that doesn't
> read out as a List at all, or it does but the elements aren't Query
> nodes, etc etc.  There's a very finite limit to how much code I'm
> willing to expend on such scenarios.  But a test for NIL list seems
> reasonable, since this function previously had what looked like sane
> handling for that case.
>
> Anyway, I adjusted Kyotaro-san's patch a bit (including fixing the
> near identical code in make_viewdef) and pushed it.

Thanks!

--
Kyotaro Horiguchi
NTT Open Source Software Center