Re: [BUGS] Server crash while trying to read expression using pg_get_expr() - Mailing list pgsql-hackers
From | Heikki Linnakangas |
---|---|
Subject | Re: [BUGS] Server crash while trying to read expression using pg_get_expr() |
Date | |
Msg-id | 4C172C43.5050309@enterprisedb.com Whole thread Raw |
In response to | Re: [BUGS] Server crash while trying to read expression using pg_get_expr() (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: [BUGS] Server crash while trying to read expression using pg_get_expr()
Re: [BUGS] Server crash while trying to read expression using pg_get_expr() |
List | pgsql-hackers |
On 10/06/10 00:24, Tom Lane wrote: > 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. Yeah, seems like it would work. You could avoid changing the meaning of fn_expr by putting the check in the parse analysis phase, into transformFuncCall(). That would feel safer at least for back-branches. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
pgsql-hackers by date: