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

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;