Re: 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(at)pgmasters(dot)net