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

From: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
To: tgl(at)sss(dot)pgh(dot)pa(dot)us
Cc: ranier(dot)vf(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Explicit NULL dereference (src/backend/utils/adt/ruleutils.c)
Date: 2020-11-02 04:36:23
Message-ID: 20201102.133623.1598224294990754677.horikyota.ntt@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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..).

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
ev_qual_and_action_catnnot_be_null_3.patch text/x-patch 1.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Yuki Seino 2020-11-02 05:02:47 Re: enable pg_stat_statements to track rows processed by REFRESH MATERIALIZED VIEW
Previous Message Nikhil Benesch 2020-11-02 03:48:26 Re: [PATCH] Support negative indexes in split_part