Re: PATCH: Configurable file mode mask

From: David Steele <david(at)pgmasters(dot)net>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Adam Brightwell <adam(dot)brightwell(at)crunchydata(dot)com>
Subject: Re: PATCH: Configurable file mode mask
Date: 2018-01-02 16:43:14
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Robert,

Thanks for looking at the patches.

On 12/31/17 1:27 PM, Robert Haas wrote:
> On Thu, Dec 28, 2017 at 2:36 PM, David Steele <david(at)pgmasters(dot)net> wrote:
>> Attached is a new patch set that should address various concerns raised in
>> this thread.
>> 1) group-access-v3-01-mkdir.patch
>> Abstracts all mkdir calls in the backend into a MakeDirectory() function
>> implemented in fd.c. This did not get committed in September as part of
>> 0c5803b450e but I still think it has value. However, I have kept it
>> separate to reduce noise in the second patch. The mkdir() calls could also
>> be modified to use PG_DIR_MODE_DEFAULT with equivalent results.
> +/*
> + * Default mode for created directories, unless something else is specified
> + * using the MakeDirectoryPerm() function variant.
> + */
> There's only one comment here, but there are two constants that do
> different things.

Looks like a copy pasto - I didn't mean PG_MODE_MASK_DEFAULT to be in
patch 01 at all. Removed.

> Also, this hunk gets removed again by 02, which is probably OK, but 02
> adds the replacement stuff in a different part of the file, which
> seems not as good.

Agreed. I have moved that.

> > I think MakeDirectory() is a good wrapper, but isn't
> MakeDirectoryPerm() sort of silly?

There's one place in the backend (storage/ipc/ipc.c) that sets
non-default directory permissions. This function is intended to support
that and any extensions that need to set custom perms.

>> 2) group-access-v3-02-group.patch
>> This is a "GUC-less" implementation of group read access that depends on the
>> mode of the $PGDATA directory to determine which mode to use for subsequent
>> writes. The initdb option is preserved to allow group access to be enabled
>> when the cluster is initialized.
>> Only two modes are allowed (700, 750) and the error message on startup is
>> hard-coded to address translation concerns.
> + umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT));
> Hmm, I wonder if this is going to be 100% portable. Maybe some
> obscure platform won't like an argument with all the high bits set.

Sure - I have masked this with 0777 to clear any high bits. Sound OK?

> Also, why should we break what's now one #if into two? That seems
> like it's mildly more complicated for no gain.

There were already two #ifdef blocks so I added a third. I think we
should leave it as three or combine them all into one. There are no
runtime performance implications so I'm agnostic.

> + (These files can confuse <application>pg_ctl</application>.) If group read
> + access is enabled on the data directory and an unprivileged user in the
> + <productname>PostgreSQL</productname> group is performing the backup, then
> + <filename></filename> will not be readable and must be
> + excluded.
> Is that actually the behavior we want?

I think it is. Comments in the code are pretty specific about not
changing the permissions on and I don't see any reason to
mess with it. Copying as part of a backup is not a good
idea anyway.

New patches attached.


Attachment Content-Type Size
group-access-v4-01-mkdir.patch text/plain 10.4 KB
group-access-v4-02-group.patch text/plain 13.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-01-02 16:47:45 Re: [HACKERS] SQL procedures
Previous Message Robert Haas 2018-01-02 16:40:26 Re: [HACKERS] SQL procedures