Re: Explicit NULL dereference (src/backend/utils/adt/ruleutils.c)

Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi <
horikyota(dot)ntt(at)gmail(dot)com> escreveu:

> At Sun, 01 Nov 2020 21:05:29 -0500, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> > Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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..).
>
I think that Assert is not the right solution here.

For a function that returns NULL twice (SPI_getvalue), it is worth testing
the result against NULL.
In the future, any modification may cause further dereference.
In addition, the static analysis tools would continue to note this snippet
either as a bug or as a suspect.

Checking "actions" pointer against NULL, and acting appropriately would do.

regards,
Ranier Vilela