Re: Explicit NULL dereference (src/backend/utils/adt/ruleutils.c) - Mailing list pgsql-hackers

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;