Re: Securing "make check" (CVE-2014-0067)

Re: Noah Misch 2014-07-12 <20140712170151(dot)GA1985627(at)tornado(dot)leadboat(dot)com>
> Thanks. Preliminary questions:
>
> > +#ifdef HAVE_UNIX_SOCKETS
> > +/* make_temp_sockdir() is invoked at most twice from pg_upgrade.c via get_sock_dir() */
> > +#define MAX_TEMPDIRS 2
> > +static int n_tempdirs = 0; /* actual number of directories created */
> > +static const char *temp_sockdir[MAX_TEMPDIRS];
> > +#endif
>
> get_sock_dir() currently returns the same directory, the CWD, for both calls;
> can't it continue to do so? We already have good reason not to start two
> postmasters simultaneously during pg_upgrade.
>
> > +/*
> > + * Remove the socket temporary directories. pg_ctl waits for postmaster
> > + * shutdown, so we expect the directory to be empty, unless we are interrupted
> > + * by a signal, in which case the postmaster will clean up the sockets, but
> > + * there's a race condition with us removing the directory.
>
> What's the reason for addressing that race condition in pg_regress and not
> addressing it in pg_upgrade?

I didn't want to have too many arrays for additionally storing the
socket and lockfile names, and unlike pg_regress, pg_upgrade usually
doesn't need to delete the files by itself, so it seemed like a good
choice to rely on the postmaster removing them.

Now, if get_sock_dir() should only return a single directory instead
of many (well, two), that makes the original code from pg_regress more
attractive to use. (Possibly it will even be a candidate for moving to
pgcommon.a, though the static/global variables might defeat that.)

I'll send an updated patch soonish.

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/