Re: sloppy back-patching of column-privilege leak - Mailing list pgsql-hackers

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> In 9.2 and newer branches, this commit makes substantial changes to
> execMain.c.  In 9.1 and 9.0, though, the change to that file is just
> this:
>
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -95,6 +95,15 @@ static void intorel_receive(TupleTableSlot *slot,
> DestReceiver *self);
>  static void intorel_shutdown(DestReceiver *self);
>  static void intorel_destroy(DestReceiver *self);
>
> +/*
> + * Note that this macro also exists in commands/trigger.c.  There does not
> + * appear to be any good header to put it into, given the structures that
> + * it uses, so we let them be duplicated.  Be sure to update both if one needs
> + * to be changed, however.
> + */
> +#define GetModifiedColumns(relinfo, estate) \
> +       (rt_fetch((relinfo)->ri_RangeTableIndex,
> (estate)->es_range_table)->modifiedCols)
> +
>  /* end of local decls */
>
> We shouldn't be adding a macro to that file that isn't used anywhere.

Ah, yes, agreed, that didn't need to be added in the older branches.

> Either that file needs more substantial changes, or it doesn't need
> this, either.  What gives?

It doesn't need it.  The older branches don't produce as detailed errors
and because of that there weren't any further changes to be made.
Including the extraneous #define in those older branches was certainly a
mistake and I'll correct it.

> Besides the sloppiness of back-patching in
> that one macro and nothing else, this is a huge fraction of the patch
> that's just *gone* in the 9.0 and 9.1 branches, and there's not so
> much as a hint about it in the commit message, which is pretty
> astonishing to me.

Right, 9.0 and 9.1 don't have as detailed messages and so there wasn't
as much to do in those older branches.  I was well aware of that and I
could have sworn that I included something in the commit messages..
Looks like I did, but not in a way which was as clear as it should have
been.  Specifically, in those older branches, the commit message only
talks about the functions which exist in those branches-
BuildIndexValueDescription and ri_ReportViolation.  The commit messages
for 9.2 and beyond also reference ExecBuildSlotValueDescription, which
is what you're getting at.

In hindsight, I agree that doing just that wasn't sufficient and
thinking on it now I realize that, while having different commit
messages for each branch may make sense if you're only looking at that
branch, it ends up being confusing for folks following the overall
project as they likely look at just the subject of each commit and
expect the contents for every branch to be the same.  To that point,
I'll try to be clearer when there's a difference in commit message for
each branch, or simply include everything for every branch in an
identical commit message across all of them.

> Notice how the comments match the actual behavior.  But in 9.0, you've
> got the SAME COMMENTS with DIFFERENT BEHAVIOR:

Good point.  The comments clearly were for the master-based behavior but
the older branches don't have the same detail and so they should have
been updated.  I knew that they were different, and understood and
expected those differences, made the specific changes which were
appropriate for each branch, but missed updating the comments.

> The comments say that you'll see the relevant columns, but you don't.
> Since when is it OK to check things into our regression tests where
> the comments say one thing and the behavior is something else?

I'm a bit confused by this- I certainly didn't intend this mistake to be
implying that our policy on comments and the code they're associated
with should be changed to allow that they don't match up, nor do I
advocate such a position now.

> I don't even know what to say about this.  I cannot understand how you
> can back-patch something like this and fail to notice that the
> regression tests give different output on different branches, and that
> that output is inconsistent with the comments.

I was quite aware that the regression tests were different on different
branches, as I knew (which the commit messages show) that only a subset
of the patch was relevant for the older branches since they didn't
include the detail where the security issue originated from.  I
certainly reviewed the regression tests but obviously missed that the
comments ended up out-dated due to the back-branch capabilities.

Thanks for the review and comments.  I'll remove the extraneous #define,
the comment change which was made to the other #define (as it's not
relevant since the #define is only located in one place) and fix the
regression test comments to match the behavior in the older branches.
Thanks again!
    Stephen