Re: run GUC check hooks on RESET

Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Feb 11, 2012 at 1:36 PM, Kevin Grittner
> <Kevin.Grittner@wicourts.gov> wrote:
>> "Kevin Grittner"  wrote:
>> Tom Lane wrote:
>>
>>>> I agree it's a bug that you can do what Kevin's example shows.
>>>
>>> I'll look at it and see if I can pull together a patch.
>>
>> Attached.
>>
>> Basically, if a GUC has a check function, this patch causes it to
>> be run on a RESET just like it is on a SET, to make sure that the
>> resulting value is valid to set within the context.  Some
>> messages needed adjustment.  While I was there, I made cod a
>> little more consistent among related GUCs.
>>
>> I also added a little to the regression tests to cover this.
> 
> This patch makes me a little nervous, because the existing
> behavior seems to have been coded for quite deliberately.
It does, although I'm not clear *why* it was.  I suspect it may have
been based on an assumption that whatever value is in the reset_val
field had to have been already determined to be good, so it was a
waste of cycles to check it again -- without considering that the
validity of making a change might depend on context.
> Sadly, I don't see any comments explaining why the RESET case was
> excluded originally.
That is unfortunate.  I guess it points out the value of adding a
comment to point out why we would want to check these values even on
a reset to a previously-used value.
> On the other hand, I can't see what it would break, either.  Have
> you gone through all the check hooks and verified that we're not
> violating any of their assumptions?
I studied the code enough to be convinced that the patch as it
stands can't break a check hook which only validates the value
and/or changes the value to a canonical form.  There appear to be 34
check hooks, and I reviewed the 10 in the variable.c file, although
not at great depth.  I could set aside some time this weekend to
look at all of them, in depth, if you think that is warranted.  I do
think that a check hook would have to be doing something which is
probably more appropriate for an assign hook to cause trouble, but I
can't swear that that isn't happening without spending about a full
day in reviewing it.
> I assume that you're thinking we'd only fix this in master?
Without this, I don't think it's possible for someone to enforce
protection of their data through SSI in an ironclad way.  So there
is at least some case to be made to take it back as far as 9.1.  I
don't think it makes sense to take it further, because read-only was
broken in other ways before 9.1, and I'm not aware of specific
threats further back.  On the other hand, it is a change in behavior
with at least some chance to break code which is functioning as
intended, so it's a pretty marginal candidate for back-patching from
that point of view.  I don't think a decision either way on that
would be crazy.  Personally I would hope to see it included in a 9.1
patch, perhaps after some "settling time" on master, but totally
understand if the consensus is to just patch master.
-Kevin