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

On 23/06/10 21:36, Robert Haas wrote:
> On Mon, Jun 21, 2010 at 7:50 PM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> On 15/06/10 10:31, Heikki Linnakangas wrote:
>>>
>>> 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.
>>
>> Here's a patch using that approach.
>>
>> I grepped through PostgreSQL and pgadmin source code to find the system
>> columns where valid node-strings are stored:
>>
>> pg_index.indexprs
>> pg_index.indprep
>> pg_attrdef.adbin
>> pg_proc.proargdefaults
>> pg_constraint.conbin
>>
>> Am I missing anything?
>
> I think that pg_type.typdefaultbin is used by pg_dump.

Yep, added that.

> pg_rewrite.ev_qual, pg_rewrite.ev_action, pg_trigger.tgqual also
> contain nodeToString() output but I didn't have any luck using them
> with pg_get_expr() so maybe they don't need to be included.

I left them out.

> The only other thing I notice is that, obviously, the FIXME comment
> needs to be FIXMEd before commit.

Fixed.

> I'd still be in favor of inserting at least some basic error checks
> into readfuncs.c, though just in HEAD. The restrictions implemented
> here seem adequate to prevent a security vulnerability, but superusers
> can still invoke those functions manually, and while superusers can
> clearly crash the system in any number of ways, that doesn't seem (to
> me) like an adequate justification for ignoring the return value of
> strtok(). YMMV, of course.

Agreed. I'll do that as a separate patch.

Thanks for the review!

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com