|From:||David Steele <david(at)pgmasters(dot)net>|
|To:||Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|Subject:||Re: OpenFile() Permissions Refactor|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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 |
> + 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
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)
>> errmsg("data directory \"%s\" has group or world access",
>> - errdetail("Permissions should be u=rwx (0700).")));
>> + errdetail("Permissions should be (%04o).",
>> 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
> 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
Patch v2 is attached.
|Next Message||Andreas Joseph Krogh||2017-09-13 14:31:09||Re: Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)|
|Previous Message||Pavel Stehule||2017-09-13 14:18:10||Re: psql: new help related to variables are not too readable|