Re: Add on_trusted_init and on_untrusted_init to plperl [PATCH]

On Thu, Jan 28, 2010 at 11:54:08AM -0500, Tom Lane wrote:
> Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> writes:
> > I think the only controversial change is this one:
>
> >> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
> >> Both are PGC_USERSET.
> >> SPI functions are not available when the code is run.
> >> Errors are detected and reported as ereport(ERROR, ...)
> > + plperl.on_trusted_init runs inside the Safe compartment.
>
> Isn't it a security hole if on_trusted_init is USERSET? That means
> an unprivileged user can determine what will happen in plperlu.
> SUSET would be saner.

Yes. Good point (though ITYM on_UNtrusted_init). I'll change that.

> > As I recall, Tom had concerns over the combination of PGC_USERSET and
> > before-first-use semantics.
>
> IIRC, what I was unhappy about was exposing shared library load as a
> time when interesting-to-the-user things would happen. It looks like
> you have got rid of that and instead made it happen at the first call
> of a plperl or plperlu function (respectively). That seems like a
> fairly reasonable way to work, and if it's defined that way, there
> doesn't seem any reason not to allow them to be USERSET/SUSET.

Great.

> But it ought to be documented like that, not with vague phrases like
> "when the interpreter is initialized" --- that means nothing to users.

I'll change that.

> One issue that ought to be mentioned is what about transaction
> rollback. One could argue that if the first plperl call is inside
> a transaction that later rolls back, then all the side effects of that
> should be undone. But we have no way to undo whatever happened inside
> perl, and at least in most likely uses of on_init there wouldn't
> be any advantage in trying to force a redo anyway. I think it's okay
> to just define it as happening once regardless of any transaction
> failure (but of course this had better be documented).

Will do.

Once the previous patch lands I'll post an update to this patch with
those changes applied.

Thanks.

Tim.