Re: [HACKERS] OpenFile() Permissions Refactor

Hi Peter,

Here's a new patch based on your review.  Where I had a question I made
a choice as described below:

On 9/1/17 1:58 PM, David Steele wrote:
> On 9/1/17 1:15 PM, Peter Eisentraut wrote:
>> On 8/29/17 12:15, David Steele wrote:
>>
>> I wonder whether we even need that much flexibility.  We already set a
>> global umask, so we could just open all files with 0666 and let the
>> umask sort it out.  Then we don't need all the *Perm() variants.
> 
> Well, there's one instance where the *Perm is used:
> 
> diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
> -    fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY,
> -                           S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> +    fd = OpenTransientFilePerm(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC |
> PG_BINARY,
> +                               S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> 
> I also think it's worth leaving the variants for extensions to use.
> Even though there are no calls in the core extensions it's hard to say
> what might be out there in the field.

These have been left in.

>> I don't like the function-like macros in fd.h.  We can use proper functions.
> 
> I had them as functions originally but thought macros might be
> friendlier with compilers that don't inline.  I'm happy to change them back.

Macros have been converted to functions.

>> I also wonder whether the umask save-and-restore code in copy.c and
>> be-fsstubs.c is sound.  If the AllocateFile() call in between errors
>> out, then there is nothing that restores the original umask.  This might
>> need a TRY/CATCH block, or maybe just a chmod() afterwards.
> 
> Unless I'm mistaken this is a preexisting issue.  Would you prefer I
> submit a different patch for that or combine it into this patch?
> 
> The chmod() implementation seems the safer option to me and produces
> fewer code paths.  It also prevents partially-written files from being
> accessible to any user but postgres.

I went with chmod().  The fix is incorporated in this patch but if you
want it broken out let me know.

>> The mkdir() calls could perhaps use some further refactoring so you
>> don't have to specify the mode everywhere.
> 
> I thought about that but feared it would be considered an overreach.
> Does fd.c seem like a good place for the new function?

New functions MakeDirectory() and MakeDirectoryPerm() have been added to
fd.c.

MakeDirectoryPerm() is used in ipc.c.

>> This kind of code:
>>
>> -   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
>> +   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
>>         ereport(FATAL,
>>                 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>>                  errmsg("data directory \"%s\" has group or world access",
>>                         DataDir),
>> -                errdetail("Permissions should be u=rwx (0700).")));
>> +                errdetail("Permissions should be (%04o).",
>> PG_DIR_MODE_DEFAULT)));
>>
>> can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
>> PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
>> consistent.
> 
> Well, the eventual goal is to make the mask/mode configurable - at least
> to the extent that group access is allowed.  However, I'm happy to leave
> that discussion for another day.

Changes to postmaster.c have been reverted (except to rename mkdir to
MakeDirectory).

Patch v2 is attached.

-- 
-David
david@pgmasters.net

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers