Re: OpenFile() Permissions Refactor

On 8/29/17 12:15, David Steele wrote:
> While working on the patch to allow group reads of $PGDATA I refactored
> the various OpenFile() functions to use default/global permissions
> rather than permissions defined in each call.
>
> I think the patch stands on its own regardless of whether we accept the
> patch to allow group permissions (which won't make this CF). We had a
> couple different ways of defining permissions (e.g. 0600 vs S_IRUSR |
> S_IWUSR) and in any case it represented quite a bit of duplication.
> This way seems simpler and less error-prone.

Yeah, it would be good to clean some of that up.

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.

I don't like the function-like macros in fd.h. We can use proper 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.

The mkdir() calls could perhaps use some further refactoring so you
don't have to specify the mode everywhere.

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.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services