Re: [BUGS] Server crash while trying to read expression using pg_get_expr()

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Kris Jurka <books(at)ejurka(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Rushabh Lathia <rushabh(dot)lathia(at)enterprisedb(dot)com>
Subject: Re: [BUGS] Server crash while trying to read expression using pg_get_expr()
Date: 2010-06-09 21:24:28
Message-ID: 1521.1276118668@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

I wrote:
> [ thinks for awhile... ] I wonder whether there is any way of locking
> down pg_get_expr so that it throws an error if called with anything
> except a suitable field from one of the system catalogs.

I did a bit of research into this idea. It looks at least somewhat
feasible:

* All PG versions back to 7.4 will pass the calling expression tree via
fcinfo->flinfo->fn_expr. Lack of that would be a showstopper, because
we can't change the FmgrInfo struct in back branches (ABI break). So we
can get the arguments and determine whether they are Vars.

* To determine which table a Var actually refers to, we must have access
to the rangetable, and in join cases we also need access to the plan
tree node containing the Var (since we have to drill down to the plan
node's inputs to resolve OUTER and INNER references). The rangetable is
reachable from the PlanState node, so it's sufficient to make that one
pointer available to functions. The obvious way to handle this is to
add a field to FmgrInfo, and I would suggest doing it that way as of
9.0. In the back branches, we could probably hack it without an ABI
break by having fn_expr point to some intermediate node type that
contains both the actual expression tree and the PlanState pointer
(probably, we'd make it point to FuncExprState instead of directly to
the FuncExpr, and then add the field to FuncExprState, which is far less
dangerous than redefining struct FmgrInfo). Now this depends on the
assumption that no external functions are doing anything with fn_expr
except passing it to the related fmgr.c routines, which we would teach
about this convention.

* Once we've got the above data it's a simple matter of adapting the
Var-interpretation logic used by EXPLAIN in order to find out the table
OID and column number, if any, represented by the Var. I'd suggest
adding such functions in fmgr.c to extend the API currently offered by
get_fn_expr_argtype() and friends. It seems possible that other
functions besides pg_get_expr might have use for such a capability.

What I'm suggesting is definitely not a trivial patch, and I would never
consider back-patching it if it weren't a security matter. But I think
it's doable and we'd be fixing the hole with a determinate amount of
work, whereas trying to verify the validity of pg_get_expr input
directly would be a never-ending source of more security bugs.

Comments?

regards, tom lane

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Tom Lane 2010-06-09 21:34:34 Re: [BUGS] Server crash while trying to read expression using pg_get_expr()
Previous Message Robert Haas 2010-06-09 20:48:00 Re: Invalid YAML output from EXPLAIN

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2010-06-09 21:34:34 Re: [BUGS] Server crash while trying to read expression using pg_get_expr()
Previous Message Robert Haas 2010-06-09 21:02:12 parser handling of large object OIDs