Thread: split postmaster's checkDataDir to src/common

Here's a patch to move src/port/pgcheckdir.c to src/common/checkdir.c
and add the shareable part of postmaster's current checkDataDir code
into it, as pg_check_dir_permissions.  This is in the spirit of using it
in pgstat to check the directory defined by stats_temp_directory.

(This is basically the same patch I posted earlier, except the file is
now in src/common instead of src/port.)

Barring objections I intend to push this to 9.3 and HEAD shortly.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Here's a patch to move src/port/pgcheckdir.c to src/common/checkdir.c
> and add the shareable part of postmaster's current checkDataDir code
> into it, as pg_check_dir_permissions.  This is in the spirit of using it
> in pgstat to check the directory defined by stats_temp_directory.

> (This is basically the same patch I posted earlier, except the file is
> now in src/common instead of src/port.)

Okay for HEAD, but ...

> Barring objections I intend to push this to 9.3 and HEAD shortly.

What exactly is the argument for pushing this into 9.3?  Since we
are past rc1, we should treat that branch as released.  If you wouldn't
back-patch into all supported branches, you shouldn't be patching 9.3
either.
        regards, tom lane

Tom Lane wrote:

> What exactly is the argument for pushing this into 9.3?  Since we
> are past rc1, we should treat that branch as released.  If you wouldn't
> back-patch into all supported branches, you shouldn't be patching 9.3
> either.

This is to fix the stats_temp_directory issue that the directory is too
public.  We had agreed that it wasn't a problem before 9.3, but it is in
that branch because we changed the code to write so many files now
instead of just one.

If there's agreement to tighten the permission check in HEAD only, then
I will only commit to that branch.  But it seems a bit odd to me
postpone the tightening.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> What exactly is the argument for pushing this into 9.3?  Since we
>> are past rc1, we should treat that branch as released.  If you wouldn't
>> back-patch into all supported branches, you shouldn't be patching 9.3
>> either.

> This is to fix the stats_temp_directory issue that the directory is too
> public.  We had agreed that it wasn't a problem before 9.3, but it is in
> that branch because we changed the code to write so many files now
> instead of just one.

To my mind, the new worry about 9.3 was the sloppy file deletion code,
which we've already cleaned up.  The remaining argument for tightening
the directory ownership/permissions checking was a security issue: should
we allow a risk that random people could see or alter the stats files.
That's not different in 9.3 than it was before; splitting up the data
into multiple files doesn't seem (to me) to change that risk any.

In fact, I thought that the end of the discussion left us with doubts
about whether we wanted to change this at all.  Andres pointed out for
instance that we might need a lock file in the directory to prevent
multiple PG instances from trying to share one temp directory.  I don't
know that we need to go that far, but if we don't think we need to defend
against that case, do we need defenses here at all?  In the end this is a
"you break it, you get to keep both pieces" issue, with not all that
severe consequences even if the DBA does mess it up.  There are other
subdirectories, such as pg_xlog, that we also allow people to relocate,
but for which the consequences of a bad config would be far worse than
they are for the stats dir.  We don't go out of our way to prevent
config foulups for other subdirectories, so why this one?
        regards, tom lane