Re: Assertion failure in base backup code path

On Tue, Jan 7, 2014 at 5:43 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2014-01-07 17:40:07 +0100, Magnus Hagander wrote:
> > On Tue, Dec 24, 2013 at 1:24 PM, Andres Freund <andres(at)2ndquadrant(dot)com
> >wrote:
> >
> > > On 2013-12-23 18:28:51 +0100, Magnus Hagander wrote:
> > > > On Dec 19, 2013 12:06 AM, "Andres Freund" <andres(at)2ndquadrant(dot)com>
> > > wrote:
> > > > >
> > > > > Hi Magnus,
> > > > >
> > > > > It looks to me like the path to do_pg_start_backup() outside of a
> > > > > transaction context comes from your initial commit of the base
> backup
> > > > > facility.
> > > > > The problem is that you're not allowed to do anything leading to a
> > > > > syscache/catcache lookup in those contexts.
> > > >
> > > > I think that may have come with the addition of the replication
> privilege
> > > > actually but that doesn't change the fact that yes, it appears
> broken..
> > >
> > > There was a if (!superuser()) check there before as well, that has the
> > > same dangers.
> > >
> > >
> > I think the correct fix is to move the security check outside of
> > do_pg_start_backup() and leave it to the caller. That means
> > pg_start_backup() for a manual backup. And for a streaming base backup
> the
> > check has already been made - you can't get through the authentication
> step
> > if you don't have the required permissions.
>
> Yes, that's what I was thinking and was planning to write you about at
> some point.
>

Good, then we think the same :)

> Does the attached seem right to you?
>
> I haven't run the code, but it looks right to me.
>
>
It worked fine in my testing, so I've pushed that version.

Looks slightly different in both 9.2 and 9.1 (not clean backpatching) due
to code reorganization and such, but AFAICT it's just code in different
places.

Thanks!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/