Re: OpenFile() Permissions Refactor

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.

> 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.

> 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.

> 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?

> 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.

Thanks,
--
-David
david(at)pgmasters(dot)net