Re: OpenFile() Permissions Refactor

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> 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.

-1. What that would mean is that if somebody had a need for a nonstandard
file mask, the path of least resistance would be to bypass fd.c and open
the file directly. We do *not* want to encourage that.

I think David's design with macros inserting the default permission
choices looks fine (but I haven't read the patch completely).

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

Yeah, this seems like a problem.

> 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)));

I think this hunk should be left entirely alone. The goal of this
patch should not be "eliminate every reference to S_IRWXO", anyway.
Trying to replace this one seems like it can have no good effect:
it would mean that if anyone ever changes PG_MODE_MASK_DEFAULT,
they've silently introduced either a security hole or a overly
restrictive check.

regards, tom lane