OpenFile() Permissions Refactor
Started by David Steeleover 8 years ago7 messages
Hackers,
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.
I have intentionally not touched the open/fopen/mkdir calls in these
files since they are specialized and require per-instance consideration
(if they are changed at all):
backend/postmaster/fork_process.c
backend/postmaster/postmaster.c
backend/utils/error/elog.c
backend/utils/init/miscinit.c
All tests pass but it's possible that I've missed something or changed
something that shouldn't be changed.
I'll submit the patch to the 2017-09 CF.
Thanks,
--
-David
david@pgmasters.net
Attachments:
file-open-perm-refactor-v1.patchtext/plain; charset=UTF-8; name=file-open-perm-refactor-v1.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index fa409d72b7..3ab1fd2db4 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1869,8 +1869,7 @@ qtext_store(const char *query, int query_len,
*query_offset = off;
/* Now write the data into the successfully-reserved part of the file */
- fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY);
if (fd < 0)
goto error;
@@ -1934,7 +1933,7 @@ qtext_load_file(Size *buffer_size)
int fd;
struct stat stat;
- fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index bd560e47e1..f93c194e18 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1013,8 +1013,7 @@ logical_rewrite_log_mapping(RewriteState state, TransactionId xid,
src->off = 0;
memcpy(src->path, path, sizeof(path));
src->vfd = PathNameOpenFile(path,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (src->vfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -1133,8 +1132,7 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
xlrec->mapped_xid, XLogRecGetXid(r));
fd = OpenTransientFile(path,
- O_CREAT | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_WRONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -1258,7 +1256,7 @@ CheckPointLogicalRewriteHeap(void)
}
else
{
- int fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ int fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
/*
* The file cannot vanish due to concurrency since this function
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 77edc51e1c..9dd77190ec 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -599,7 +599,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
SlruFileName(ctl, path, segno);
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
/* expected: file doesn't exist */
@@ -654,7 +654,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
* SlruPhysicalWritePage). Hence, if we are InRecovery, allow the case
* where the file doesn't exist, and return zeroes instead.
*/
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT || !InRecovery)
@@ -804,8 +804,7 @@ SlruPhysicalWritePage(SlruCtl ctl, int pageno, int slotno, SlruFlush fdata)
* don't use O_EXCL or O_TRUNC or anything like that.
*/
SlruFileName(ctl, path, segno);
- fd = OpenTransientFile(path, O_RDWR | O_CREAT | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(path, O_RDWR | O_CREAT | PG_BINARY);
if (fd < 0)
{
slru_errcause = SLRU_OPEN_FAILED;
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 63db8a981d..3d65e5624a 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -307,8 +307,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -325,7 +324,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
else
TLHistoryFilePath(path, parentTLI);
- srcfd = OpenTransientFile(path, O_RDONLY, 0);
+ srcfd = OpenTransientFile(path, O_RDONLY);
if (srcfd < 0)
{
if (errno != ENOENT)
@@ -459,8 +458,7 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index ba03d9687e..6233bfdd35 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1195,7 +1195,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
TwoPhaseFilePath(path, xid);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (give_warnings)
@@ -1580,8 +1580,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len)
TwoPhaseFilePath(path, xid);
fd = OpenTransientFile(path,
- O_CREAT | O_TRUNC | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_TRUNC | O_WRONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index df4843f409..d1f067915c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3168,8 +3168,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
*/
if (*use_existent)
{
- fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method));
if (fd < 0)
{
if (errno != ENOENT)
@@ -3194,8 +3193,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3291,8 +3289,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
*use_existent = false;
/* Now open original target segment (might not be file I just made) */
- fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method));
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3333,7 +3330,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
* Open the source file
*/
XLogFilePath(path, srcTLI, srcsegno);
- srcfd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ srcfd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (srcfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3347,8 +3344,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3526,8 +3522,7 @@ XLogFileOpen(XLogSegNo segno)
XLogFilePath(path, ThisTimeLineID, segno);
- fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method));
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -3593,7 +3588,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlogfname);
}
- fd = BasicOpenFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
if (fd >= 0)
{
/* Success! */
@@ -4084,7 +4079,7 @@ ValidateXLOGDirectoryStructure(void)
{
ereport(LOG,
(errmsg("creating missing WAL directory \"%s\"", path)));
- if (mkdir(path, S_IRWXU) < 0)
+ if (mkdir(path, PG_DIR_MODE_DEFAULT) < 0)
ereport(FATAL,
(errmsg("could not create missing directory \"%s\": %m",
path)));
@@ -4428,8 +4423,7 @@ WriteControlFile(void)
memcpy(buffer, ControlFile, sizeof(ControlFileData));
fd = BasicOpenFile(XLOG_CONTROL_FILE,
- O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -4472,8 +4466,7 @@ ReadControlFile(void)
* Read data...
*/
fd = BasicOpenFile(XLOG_CONTROL_FILE,
- O_RDWR | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_RDWR | PG_BINARY);
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -4654,8 +4647,7 @@ UpdateControlFile(void)
FIN_CRC32C(ControlFile->crc);
fd = BasicOpenFile(XLOG_CONTROL_FILE,
- O_RDWR | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_RDWR | PG_BINARY);
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index bbae733d65..0eaba71ea8 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -691,7 +691,7 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
XLogFilePath(path, tli, sendSegNo);
- sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY, 0);
+ sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY);
if (sendFile < 0)
{
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 92d943cac7..f50ae3e41d 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -444,7 +444,7 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
/* Check for existing file of same name */
rpath = relpath(rnode, MAIN_FORKNUM);
- fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY, 0);
+ fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY);
if (fd >= 0)
{
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 8559c3b6b3..e6a94a5392 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -151,7 +151,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
else
{
/* Directory creation failed? */
- if (mkdir(dir, S_IRWXU) < 0)
+ if (mkdir(dir, PG_DIR_MODE_DEFAULT) < 0)
{
char *parentdir;
@@ -173,7 +173,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
get_parent_directory(parentdir);
get_parent_directory(parentdir);
/* Can't create parent and it doesn't already exist? */
- if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST)
+ if (mkdir(parentdir, PG_DIR_MODE_DEFAULT) < 0 && errno != EEXIST)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -184,7 +184,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
parentdir = pstrdup(dir);
get_parent_directory(parentdir);
/* Can't create parent and it doesn't already exist? */
- if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST)
+ if (mkdir(parentdir, PG_DIR_MODE_DEFAULT) < 0 && errno != EEXIST)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -192,7 +192,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
pfree(parentdir);
/* Create database directory */
- if (mkdir(dir, S_IRWXU) < 0)
+ if (mkdir(dir, PG_DIR_MODE_DEFAULT) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -610,7 +610,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
* The creation of the version directory prevents more than one tablespace
* in a single location.
*/
- if (mkdir(location_with_version_dir, S_IRWXU) < 0)
+ if (mkdir(location_with_version_dir, PG_DIR_MODE_DEFAULT) < 0)
{
if (errno == EEXIST)
ereport(ERROR,
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index bf45461b2f..44cd79403b 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -462,7 +462,7 @@ lo_import_internal(text *filename, Oid lobjOid)
* open the file to be read in
*/
text_to_cstring_buffer(filename, fnamebuf, sizeof(fnamebuf));
- fd = OpenTransientFile(fnamebuf, O_RDONLY | PG_BINARY, S_IRWXU);
+ fd = OpenTransientFile(fnamebuf, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -538,8 +538,8 @@ be_lo_export(PG_FUNCTION_ARGS)
*/
text_to_cstring_buffer(filename, fnamebuf, sizeof(fnamebuf));
oumask = umask(S_IWGRP | S_IWOTH);
- 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);
umask(oumask);
if (fd < 0)
ereport(ERROR,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..32b3832a02 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -588,7 +588,7 @@ PostmasterMain(int argc, char *argv[])
/*
* for security, no dir or file created can be group or other accessible
*/
- umask(S_IRWXG | S_IRWXO);
+ umask(PG_MODE_MASK_DEFAULT);
/*
* Initialize random(3) so we don't get the same values in every run.
@@ -1533,12 +1533,12 @@ checkDataDir(void)
* reasonable check to apply on Windows.
*/
#if !defined(WIN32) && !defined(__CYGWIN__)
- 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)));
#endif
/* Look for PG_VERSION before looking for pg_control */
@@ -4440,7 +4440,7 @@ internal_forkexec(int argc, char *argv[], Port *port)
* As in OpenTemporaryFileInTablespace, try to make the temp-file
* directory
*/
- mkdir(PG_TEMP_FILES_DIR, S_IRWXU);
+ mkdir(PG_TEMP_FILES_DIR, PG_DIR_MODE_DEFAULT);
fp = AllocateFile(tmpfilename, PG_BINARY_W);
if (!fp)
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 3255b42c7d..41e918b19e 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -41,6 +41,7 @@
#include "postmaster/postmaster.h"
#include "postmaster/syslogger.h"
#include "storage/dsm.h"
+#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/latch.h"
#include "storage/pg_shmem.h"
@@ -322,7 +323,7 @@ SysLoggerMain(int argc, char *argv[])
/*
* Also, create new directory if not present; ignore errors
*/
- mkdir(Log_directory, S_IRWXU);
+ mkdir(Log_directory, PG_DIR_MODE_DEFAULT);
}
if (strcmp(Log_filename, currentLogFilename) != 0)
{
@@ -564,7 +565,7 @@ SysLogger_Start(void)
/*
* Create log directory if not present; ignore errors
*/
- mkdir(Log_directory, S_IRWXU);
+ mkdir(Log_directory, PG_DIR_MODE_DEFAULT);
/*
* The initial logfile is created right in the postmaster, to verify that
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 14cb3d0bf2..4b4ae278da 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -547,8 +547,7 @@ CheckPointReplicationOrigin(void)
* CheckpointLock.
*/
tmpfd = OpenTransientFile((char *) tmppath,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (tmpfd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -660,7 +659,7 @@ StartupReplicationOrigin(void)
elog(DEBUG2, "starting up replication origin progress state");
- fd = OpenTransientFile((char *) path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile((char *) path, O_RDONLY | PG_BINARY);
/*
* might have had max_replication_slots == 0 last run, or we just brought
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 657bafae57..98266e0613 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2103,8 +2103,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* open segment, create it if necessary */
fd = OpenTransientFile(path,
- O_CREAT | O_WRONLY | O_APPEND | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_WRONLY | O_APPEND | PG_BINARY);
if (fd < 0)
ereport(ERROR,
@@ -2348,7 +2347,7 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
NameStr(MyReplicationSlot->data.name), txn->xid,
(uint32) (recptr >> 32), (uint32) recptr);
- *fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ *fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (*fd < 0 && errno == ENOENT)
{
*fd = -1;
@@ -3037,7 +3036,7 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
LogicalRewriteMappingData map;
sprintf(path, "pg_logical/mappings/%s", fname);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index fba57a0470..ad65b9831d 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1597,8 +1597,7 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
/* we have valid data now, open tempfile and write it there */
fd = OpenTransientFile(tmppath,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errmsg("could not open file \"%s\": %m", path)));
@@ -1682,7 +1681,7 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
sprintf(path, "pg_logical/snapshots/%X-%X.snap",
(uint32) (lsn >> 32), (uint32) lsn);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0 && errno == ENOENT)
return false;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index a8a16f55e9..79748de0cc 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1172,7 +1172,7 @@ CreateSlotOnDisk(ReplicationSlot *slot)
rmtree(tmppath, true);
/* Create and fsync the temporary slot directory. */
- if (mkdir(tmppath, S_IRWXU) < 0)
+ if (mkdir(tmppath, PG_DIR_MODE_DEFAULT) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -1233,9 +1233,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
sprintf(tmppath, "%s/state.tmp", dir);
sprintf(path, "%s/state", dir);
- fd = OpenTransientFile(tmppath,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (fd < 0)
{
ereport(elevel,
@@ -1354,7 +1352,7 @@ RestoreSlotFromDisk(const char *name)
elog(DEBUG1, "restoring replication slot from \"%s\"", path);
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
/*
* We do not need to handle this as we are rename()ing the directory into
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 03e1cf44de..11d9385751 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -472,7 +472,7 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
pq_sendint(&buf, len, 4); /* col1 len */
pq_sendbytes(&buf, histfname, len);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0666);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -2366,7 +2366,7 @@ retry:
XLogFilePath(path, curFileTimeLine, sendSegNo);
- sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY, 0);
+ sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY);
if (sendFile < 0)
{
/*
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 1e2691685a..a2425350b6 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -41,7 +41,7 @@ copydir(char *fromdir, char *todir, bool recurse)
char fromfile[MAXPGPATH * 2];
char tofile[MAXPGPATH * 2];
- if (mkdir(todir, S_IRWXU) != 0)
+ if (mkdir(todir, PG_DIR_MODE_DEFAULT) != 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m", todir)));
@@ -148,14 +148,13 @@ copy_file(char *fromfile, char *tofile)
/*
* Open the files
*/
- srcfd = OpenTransientFile(fromfile, O_RDONLY | PG_BINARY, 0);
+ srcfd = OpenTransientFile(fromfile, O_RDONLY | PG_BINARY);
if (srcfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m", fromfile)));
- dstfd = OpenTransientFile(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ dstfd = OpenTransientFile(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (dstfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 83b061a036..052b9f638f 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -604,7 +604,7 @@ durable_rename(const char *oldfile, const char *newfile, int elevel)
if (fsync_fname_ext(oldfile, false, false, elevel) != 0)
return -1;
- fd = OpenTransientFile((char *) newfile, PG_BINARY | O_RDWR, 0);
+ fd = OpenTransientFile((char *) newfile, PG_BINARY | O_RDWR);
if (fd < 0)
{
if (errno != ENOENT)
@@ -933,7 +933,7 @@ set_max_safe_fds(void)
* this module wouldn't have any open files to close at that point anyway.
*/
int
-BasicOpenFile(FileName fileName, int fileFlags, int fileMode)
+BasicOpenFilePerm(FileName fileName, int fileFlags, int fileMode)
{
int fd;
@@ -1084,8 +1084,8 @@ LruInsert(File file)
* overall system file table being full. So, be prepared to release
* another FD if necessary...
*/
- vfdP->fd = BasicOpenFile(vfdP->fileName, vfdP->fileFlags,
- vfdP->fileMode);
+ vfdP->fd = BasicOpenFilePerm(vfdP->fileName, vfdP->fileFlags,
+ vfdP->fileMode);
if (vfdP->fd < 0)
{
DO_DB(elog(LOG, "re-open failed: %m"));
@@ -1300,13 +1300,13 @@ FileInvalidate(File file)
* (which should always be $PGDATA when this code is running).
*/
File
-PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
+PathNameOpenFilePerm(FileName fileName, int fileFlags, int fileMode)
{
char *fnamecopy;
File file;
Vfd *vfdP;
- DO_DB(elog(LOG, "PathNameOpenFile: %s %x %o",
+ DO_DB(elog(LOG, "PathNameOpenFilePerm: %s %x %o",
fileName, fileFlags, fileMode));
/*
@@ -1324,7 +1324,7 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
/* Close excess kernel FDs. */
ReleaseLruFiles();
- vfdP->fd = BasicOpenFile(fileName, fileFlags, fileMode);
+ vfdP->fd = BasicOpenFilePerm(fileName, fileFlags, fileMode);
if (vfdP->fd < 0)
{
@@ -1461,8 +1461,7 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError)
* temp file that can be reused.
*/
file = PathNameOpenFile(tempfilepath,
- O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
- 0600);
+ O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
if (file <= 0)
{
/*
@@ -1473,11 +1472,10 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError)
* just did the same thing. If it doesn't work then we'll bomb out on
* the second create attempt, instead.
*/
- mkdir(tempdirpath, S_IRWXU);
+ mkdir(tempdirpath, PG_DIR_MODE_DEFAULT);
file = PathNameOpenFile(tempfilepath,
- O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
- 0600);
+ O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
if (file <= 0 && rejectError)
elog(ERROR, "could not create temporary file \"%s\": %m",
tempfilepath);
@@ -2141,7 +2139,7 @@ TryAgain:
* Like AllocateFile, but returns an unbuffered fd like open(2)
*/
int
-OpenTransientFile(FileName fileName, int fileFlags, int fileMode)
+OpenTransientFilePerm(FileName fileName, int fileFlags, int fileMode)
{
int fd;
@@ -2158,7 +2156,7 @@ OpenTransientFile(FileName fileName, int fileFlags, int fileMode)
/* Close excess kernel FDs. */
ReleaseLruFiles();
- fd = BasicOpenFile(fileName, fileFlags, fileMode);
+ fd = BasicOpenFilePerm(fileName, fileFlags, fileMode);
if (fd >= 0)
{
@@ -3081,7 +3079,7 @@ pre_sync_fname(const char *fname, bool isdir, int elevel)
if (isdir)
return;
- fd = OpenTransientFile((char *) fname, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile((char *) fname, O_RDONLY | PG_BINARY);
if (fd < 0)
{
@@ -3141,7 +3139,7 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
else
flags |= O_RDONLY;
- fd = OpenTransientFile((char *) fname, flags, 0);
+ fd = OpenTransientFile((char *) fname, flags);
/*
* Some OSs don't allow us to open directories at all (Windows returns
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 1500465d31..3cf34dbc4f 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -835,7 +835,7 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
/* Create new segment or open an existing one for attach or resize. */
flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0);
- if ((fd = OpenTransientFile(name, flags, 0600)) == -1)
+ if ((fd = OpenTransientFilePerm(name, flags, S_IRUSR | S_IWUSR)) == -1)
{
if (errno != EEXIST)
ereport(elevel,
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 65e0abe9ec..64a4ccf0db 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -304,7 +304,7 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
path = relpath(reln->smgr_rnode, forkNum);
- fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
{
@@ -317,7 +317,7 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
* already, even if isRedo is not set. (See also mdopen)
*/
if (isRedo || IsBootstrapProcessingMode())
- fd = PathNameOpenFile(path, O_RDWR | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
/* be sure to report the error reported by create, not open */
@@ -430,7 +430,7 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
/* truncate(2) would be easier here, but Windows hasn't got it */
int fd;
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
if (fd >= 0)
{
int save_errno;
@@ -583,7 +583,7 @@ mdopen(SMgrRelation reln, ForkNumber forknum, int behavior)
path = relpath(reln->smgr_rnode, forknum);
- fd = PathNameOpenFile(path, O_RDWR | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
@@ -594,7 +594,7 @@ mdopen(SMgrRelation reln, ForkNumber forknum, int behavior)
* substitute for mdcreate() in bootstrap mode only. (See mdcreate)
*/
if (IsBootstrapProcessingMode())
- fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
{
if ((behavior & EXTENSION_RETURN_NULL) &&
@@ -1780,7 +1780,7 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
fullpath = _mdfd_segpath(reln, forknum, segno);
/* open the file */
- fd = PathNameOpenFile(fullpath, O_RDWR | PG_BINARY | oflags, 0600);
+ fd = PathNameOpenFile(fullpath, O_RDWR | PG_BINARY | oflags);
pfree(fullpath);
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index f5394dc43d..41c2ba7f97 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -644,8 +644,7 @@ load_relmap_file(bool shared)
}
/* Read data ... */
- fd = OpenTransientFile(mapfilename,
- O_RDONLY | PG_BINARY, S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(mapfilename, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(FATAL,
(errcode_for_file_access(),
@@ -745,9 +744,7 @@ write_relmap_file(bool shared, RelMapFile *newmap,
realmap = &local_map;
}
- fd = OpenTransientFile(mapfilename,
- O_WRONLY | O_CREAT | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(mapfilename, O_WRONLY | O_CREAT | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 246fea8693..18fe4eea88 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7243,8 +7243,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
* truncate and reuse it.
*/
Tmpfd = BasicOpenFile(AutoConfTmpFileName,
- O_CREAT | O_RDWR | O_TRUNC,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_RDWR | O_TRUNC);
if (Tmpfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index faef39e78d..ae9340b076 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -22,7 +22,7 @@
* Use them for all file activity...
*
* File fd;
- * fd = PathNameOpenFile("foo", O_RDONLY, 0600);
+ * fd = PathNameOpenFile("foo", O_RDONLY);
*
* AllocateFile();
* FreeFile();
@@ -59,13 +59,25 @@ extern int max_files_per_process;
*/
extern int max_safe_fds;
+/* Define default mode mask and default file/directory modes. */
+#define PG_MODE_MASK_DEFAULT (S_IRWXG | S_IRWXO)
+#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR)
+#define PG_DIR_MODE_DEFAULT (S_IRWXU)
+
+/* Macros to call *Open*Perm() functions with default file permissions. */
+#define BasicOpenFile(fileName, fileFlags) \
+ BasicOpenFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT)
+#define PathNameOpenFile(fileName, fileFlags) \
+ PathNameOpenFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT)
+#define OpenTransientFile(fileName, fileFlags) \
+ OpenTransientFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT)
/*
* prototypes for functions in fd.c
*/
/* Operations on virtual Files --- equivalent to Unix kernel file ops */
-extern File PathNameOpenFile(FileName fileName, int fileFlags, int fileMode);
+extern File PathNameOpenFilePerm(FileName fileName, int fileFlags, int fileMode);
extern File OpenTemporaryFile(bool interXact);
extern void FileClose(File file);
extern int FilePrefetch(File file, off_t offset, int amount, uint32 wait_event_info);
@@ -94,11 +106,11 @@ extern struct dirent *ReadDir(DIR *dir, const char *dirname);
extern int FreeDir(DIR *dir);
/* Operations to allow use of a plain kernel FD, with automatic cleanup */
-extern int OpenTransientFile(FileName fileName, int fileFlags, int fileMode);
+extern int OpenTransientFilePerm(FileName fileName, int fileFlags, int fileMode);
extern int CloseTransientFile(int fd);
/* If you've really really gotta have a plain kernel FD, use this */
-extern int BasicOpenFile(FileName fileName, int fileFlags, int fileMode);
+extern int BasicOpenFilePerm(FileName fileName, int fileFlags, int fileMode);
/* Miscellaneous support routines */
extern void InitFileAccess(void);
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: OpenFile() Permissions Refactor
Peter Eisentraut <peter.eisentraut@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@pgmasters.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@pgmasters.net
Attachments:
file-open-perm-refactor-v2.patchtext/plain; charset=UTF-8; name=file-open-perm-refactor-v2.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index fa409d72b7..3ab1fd2db4 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1869,8 +1869,7 @@ qtext_store(const char *query, int query_len,
*query_offset = off;
/* Now write the data into the successfully-reserved part of the file */
- fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY);
if (fd < 0)
goto error;
@@ -1934,7 +1933,7 @@ qtext_load_file(Size *buffer_size)
int fd;
struct stat stat;
- fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index bd560e47e1..f93c194e18 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1013,8 +1013,7 @@ logical_rewrite_log_mapping(RewriteState state, TransactionId xid,
src->off = 0;
memcpy(src->path, path, sizeof(path));
src->vfd = PathNameOpenFile(path,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (src->vfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -1133,8 +1132,7 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
xlrec->mapped_xid, XLogRecGetXid(r));
fd = OpenTransientFile(path,
- O_CREAT | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_WRONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -1258,7 +1256,7 @@ CheckPointLogicalRewriteHeap(void)
}
else
{
- int fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ int fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
/*
* The file cannot vanish due to concurrency since this function
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 77edc51e1c..9dd77190ec 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -599,7 +599,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
SlruFileName(ctl, path, segno);
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
/* expected: file doesn't exist */
@@ -654,7 +654,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
* SlruPhysicalWritePage). Hence, if we are InRecovery, allow the case
* where the file doesn't exist, and return zeroes instead.
*/
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT || !InRecovery)
@@ -804,8 +804,7 @@ SlruPhysicalWritePage(SlruCtl ctl, int pageno, int slotno, SlruFlush fdata)
* don't use O_EXCL or O_TRUNC or anything like that.
*/
SlruFileName(ctl, path, segno);
- fd = OpenTransientFile(path, O_RDWR | O_CREAT | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(path, O_RDWR | O_CREAT | PG_BINARY);
if (fd < 0)
{
slru_errcause = SLRU_OPEN_FAILED;
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 63db8a981d..3d65e5624a 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -307,8 +307,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -325,7 +324,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
else
TLHistoryFilePath(path, parentTLI);
- srcfd = OpenTransientFile(path, O_RDONLY, 0);
+ srcfd = OpenTransientFile(path, O_RDONLY);
if (srcfd < 0)
{
if (errno != ENOENT)
@@ -459,8 +458,7 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index ae832917ce..5c58469425 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1195,7 +1195,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
TwoPhaseFilePath(path, xid);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (give_warnings)
@@ -1580,8 +1580,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len)
TwoPhaseFilePath(path, xid);
fd = OpenTransientFile(path,
- O_CREAT | O_TRUNC | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_TRUNC | O_WRONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a3e8ce092f..cfdba80fdd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3168,8 +3168,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
*/
if (*use_existent)
{
- fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method));
if (fd < 0)
{
if (errno != ENOENT)
@@ -3194,8 +3193,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3291,8 +3289,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
*use_existent = false;
/* Now open original target segment (might not be file I just made) */
- fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method));
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3333,7 +3330,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
* Open the source file
*/
XLogFilePath(path, srcTLI, srcsegno);
- srcfd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ srcfd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (srcfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3347,8 +3344,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
unlink(tmppath);
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3526,8 +3522,7 @@ XLogFileOpen(XLogSegNo segno)
XLogFilePath(path, ThisTimeLineID, segno);
- fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),
- S_IRUSR | S_IWUSR);
+ fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method));
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -3593,7 +3588,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlogfname);
}
- fd = BasicOpenFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
if (fd >= 0)
{
/* Success! */
@@ -4084,7 +4079,7 @@ ValidateXLOGDirectoryStructure(void)
{
ereport(LOG,
(errmsg("creating missing WAL directory \"%s\"", path)));
- if (mkdir(path, S_IRWXU) < 0)
+ if (MakeDirectory(path) < 0)
ereport(FATAL,
(errmsg("could not create missing directory \"%s\": %m",
path)));
@@ -4428,8 +4423,7 @@ WriteControlFile(void)
memcpy(buffer, ControlFile, sizeof(ControlFileData));
fd = BasicOpenFile(XLOG_CONTROL_FILE,
- O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -4472,8 +4466,7 @@ ReadControlFile(void)
* Read data...
*/
fd = BasicOpenFile(XLOG_CONTROL_FILE,
- O_RDWR | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_RDWR | PG_BINARY);
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -4654,8 +4647,7 @@ UpdateControlFile(void)
FIN_CRC32C(ControlFile->crc);
fd = BasicOpenFile(XLOG_CONTROL_FILE,
- O_RDWR | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_RDWR | PG_BINARY);
if (fd < 0)
ereport(PANIC,
(errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index bbae733d65..0eaba71ea8 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -691,7 +691,7 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
XLogFilePath(path, tli, sendSegNo);
- sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY, 0);
+ sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY);
if (sendFile < 0)
{
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 92d943cac7..f50ae3e41d 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -444,7 +444,7 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
/* Check for existing file of same name */
rpath = relpath(rnode, MAIN_FORKNUM);
- fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY, 0);
+ fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY);
if (fd >= 0)
{
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 8559c3b6b3..6350f5b459 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -151,7 +151,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
else
{
/* Directory creation failed? */
- if (mkdir(dir, S_IRWXU) < 0)
+ if (MakeDirectory(dir) < 0)
{
char *parentdir;
@@ -173,7 +173,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
get_parent_directory(parentdir);
get_parent_directory(parentdir);
/* Can't create parent and it doesn't already exist? */
- if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST)
+ if (MakeDirectory(parentdir) < 0 && errno != EEXIST)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -184,7 +184,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
parentdir = pstrdup(dir);
get_parent_directory(parentdir);
/* Can't create parent and it doesn't already exist? */
- if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST)
+ if (MakeDirectory(parentdir) < 0 && errno != EEXIST)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -192,7 +192,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
pfree(parentdir);
/* Create database directory */
- if (mkdir(dir, S_IRWXU) < 0)
+ if (MakeDirectory(dir) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -279,7 +279,7 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
/*
* Check that location isn't too long. Remember that we're going to append
* 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'. FYI, we never actually
- * reference the whole path here, but mkdir() uses the first two parts.
+ * reference the whole path here, but MakeDirectory() uses the first two parts.
*/
if (strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 +
OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1 + OIDCHARS > MAXPGPATH)
@@ -574,7 +574,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
* Attempt to coerce target directory to safe permissions. If this fails,
* it doesn't exist or has the wrong owner.
*/
- if (chmod(location, S_IRWXU) != 0)
+ if (chmod(location, PG_DIR_MODE_DEFAULT) != 0)
{
if (errno == ENOENT)
ereport(ERROR,
@@ -599,7 +599,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
if (stat(location_with_version_dir, &st) == 0 && S_ISDIR(st.st_mode))
{
if (!rmtree(location_with_version_dir, true))
- /* If this failed, mkdir() below is going to error. */
+ /* If this failed, MakeDirectory() below is going to error. */
ereport(WARNING,
(errmsg("some useless files may be left behind in old database directory \"%s\"",
location_with_version_dir)));
@@ -610,7 +610,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
* The creation of the version directory prevents more than one tablespace
* in a single location.
*/
- if (mkdir(location_with_version_dir, S_IRWXU) < 0)
+ if (MakeDirectory(location_with_version_dir) < 0)
{
if (errno == EEXIST)
ereport(ERROR,
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index bf45461b2f..e433446922 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -462,7 +462,7 @@ lo_import_internal(text *filename, Oid lobjOid)
* open the file to be read in
*/
text_to_cstring_buffer(filename, fnamebuf, sizeof(fnamebuf));
- fd = OpenTransientFile(fnamebuf, O_RDONLY | PG_BINARY, S_IRWXU);
+ fd = OpenTransientFile(fnamebuf, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -531,16 +531,9 @@ be_lo_export(PG_FUNCTION_ARGS)
/*
* open the file to be written to
- *
- * Note: we reduce backend's normal 077 umask to the slightly friendlier
- * 022. This code used to drop it all the way to 0, but creating
- * world-writable export files doesn't seem wise.
*/
text_to_cstring_buffer(filename, fnamebuf, sizeof(fnamebuf));
- oumask = umask(S_IWGRP | S_IWOTH);
- fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY,
- S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
- umask(oumask);
+ fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -563,6 +556,18 @@ be_lo_export(PG_FUNCTION_ARGS)
CloseTransientFile(fd);
inv_close(lobj);
+ /*
+ * Allow group read. This code used make export files world-writable but that
+ * doesn't seem wise.
+ */
+ if (chmod(fnamebuf, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0)
+ {
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not set permissions on server file \"%s\": %m",
+ fnamebuf)));
+ }
+
PG_RETURN_INT32(1);
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..11d00b854a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4440,7 +4440,7 @@ internal_forkexec(int argc, char *argv[], Port *port)
* As in OpenTemporaryFileInTablespace, try to make the temp-file
* directory
*/
- mkdir(PG_TEMP_FILES_DIR, S_IRWXU);
+ MakeDirectory(PG_TEMP_FILES_DIR);
fp = AllocateFile(tmpfilename, PG_BINARY_W);
if (!fp)
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 3255b42c7d..f58fe8e8f1 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -41,6 +41,7 @@
#include "postmaster/postmaster.h"
#include "postmaster/syslogger.h"
#include "storage/dsm.h"
+#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/latch.h"
#include "storage/pg_shmem.h"
@@ -322,7 +323,7 @@ SysLoggerMain(int argc, char *argv[])
/*
* Also, create new directory if not present; ignore errors
*/
- mkdir(Log_directory, S_IRWXU);
+ MakeDirectory(Log_directory);
}
if (strcmp(Log_filename, currentLogFilename) != 0)
{
@@ -564,7 +565,7 @@ SysLogger_Start(void)
/*
* Create log directory if not present; ignore errors
*/
- mkdir(Log_directory, S_IRWXU);
+ MakeDirectory(Log_directory);
/*
* The initial logfile is created right in the postmaster, to verify that
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index edc6efb8a6..edd539aa7a 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -547,8 +547,7 @@ CheckPointReplicationOrigin(void)
* CheckpointLock.
*/
tmpfd = OpenTransientFile((char *) tmppath,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (tmpfd < 0)
ereport(PANIC,
(errcode_for_file_access(),
@@ -660,7 +659,7 @@ StartupReplicationOrigin(void)
elog(DEBUG2, "starting up replication origin progress state");
- fd = OpenTransientFile((char *) path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile((char *) path, O_RDONLY | PG_BINARY);
/*
* might have had max_replication_slots == 0 last run, or we just brought
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 657bafae57..98266e0613 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2103,8 +2103,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* open segment, create it if necessary */
fd = OpenTransientFile(path,
- O_CREAT | O_WRONLY | O_APPEND | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_WRONLY | O_APPEND | PG_BINARY);
if (fd < 0)
ereport(ERROR,
@@ -2348,7 +2347,7 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
NameStr(MyReplicationSlot->data.name), txn->xid,
(uint32) (recptr >> 32), (uint32) recptr);
- *fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ *fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (*fd < 0 && errno == ENOENT)
{
*fd = -1;
@@ -3037,7 +3036,7 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
LogicalRewriteMappingData map;
sprintf(path, "pg_logical/mappings/%s", fname);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index fba57a0470..ad65b9831d 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1597,8 +1597,7 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
/* we have valid data now, open tempfile and write it there */
fd = OpenTransientFile(tmppath,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errmsg("could not open file \"%s\": %m", path)));
@@ -1682,7 +1681,7 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
sprintf(path, "pg_logical/snapshots/%X-%X.snap",
(uint32) (lsn >> 32), (uint32) lsn);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0 && errno == ENOENT)
return false;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index a8a16f55e9..f44e2e0d8f 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1166,13 +1166,13 @@ CreateSlotOnDisk(ReplicationSlot *slot)
* It's just barely possible that some previous effort to create or drop a
* slot with this name left a temp directory lying around. If that seems
* to be the case, try to remove it. If the rmtree() fails, we'll error
- * out at the mkdir() below, so we don't bother checking success.
+ * out at the MakeDirectory() below, so we don't bother checking success.
*/
if (stat(tmppath, &st) == 0 && S_ISDIR(st.st_mode))
rmtree(tmppath, true);
/* Create and fsync the temporary slot directory. */
- if (mkdir(tmppath, S_IRWXU) < 0)
+ if (MakeDirectory(tmppath) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m",
@@ -1233,9 +1233,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
sprintf(tmppath, "%s/state.tmp", dir);
sprintf(path, "%s/state", dir);
- fd = OpenTransientFile(tmppath,
- O_CREAT | O_EXCL | O_WRONLY | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(tmppath, O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
if (fd < 0)
{
ereport(elevel,
@@ -1354,7 +1352,7 @@ RestoreSlotFromDisk(const char *name)
elog(DEBUG1, "restoring replication slot from \"%s\"", path);
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
/*
* We do not need to handle this as we are rename()ing the directory into
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 1fbe8ed71b..b3b1cc8d49 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -472,7 +472,7 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
pq_sendint(&buf, len, 4); /* col1 len */
pq_sendbytes(&buf, histfname, len);
- fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0666);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -2366,7 +2366,7 @@ retry:
XLogFilePath(path, curFileTimeLine, sendSegNo);
- sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY, 0);
+ sendFile = BasicOpenFile(path, O_RDONLY | PG_BINARY);
if (sendFile < 0)
{
/*
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 1e2691685a..f3d2b003aa 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -41,7 +41,7 @@ copydir(char *fromdir, char *todir, bool recurse)
char fromfile[MAXPGPATH * 2];
char tofile[MAXPGPATH * 2];
- if (mkdir(todir, S_IRWXU) != 0)
+ if (MakeDirectory(todir) != 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory \"%s\": %m", todir)));
@@ -148,14 +148,13 @@ copy_file(char *fromfile, char *tofile)
/*
* Open the files
*/
- srcfd = OpenTransientFile(fromfile, O_RDONLY | PG_BINARY, 0);
+ srcfd = OpenTransientFile(fromfile, O_RDONLY | PG_BINARY);
if (srcfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m", fromfile)));
- dstfd = OpenTransientFile(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ dstfd = OpenTransientFile(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (dstfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 83b061a036..e47fdceedd 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -604,7 +604,7 @@ durable_rename(const char *oldfile, const char *newfile, int elevel)
if (fsync_fname_ext(oldfile, false, false, elevel) != 0)
return -1;
- fd = OpenTransientFile((char *) newfile, PG_BINARY | O_RDWR, 0);
+ fd = OpenTransientFile((char *) newfile, PG_BINARY | O_RDWR);
if (fd < 0)
{
if (errno != ENOENT)
@@ -917,6 +917,16 @@ set_max_safe_fds(void)
}
/*
+ * Open a file with BasicOpenFilePerm() and pass PG_FILE_MODE_DEFAULT to the
+ * fileMode parameter.
+ */
+int
+BasicOpenFile(FileName fileName, int fileFlags)
+{
+ return BasicOpenFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT);
+}
+
+/*
* BasicOpenFile --- same as open(2) except can free other FDs if needed
*
* This is exported for use by places that really want a plain kernel FD,
@@ -933,7 +943,7 @@ set_max_safe_fds(void)
* this module wouldn't have any open files to close at that point anyway.
*/
int
-BasicOpenFile(FileName fileName, int fileFlags, int fileMode)
+BasicOpenFilePerm(FileName fileName, int fileFlags, int fileMode)
{
int fd;
@@ -1084,8 +1094,8 @@ LruInsert(File file)
* overall system file table being full. So, be prepared to release
* another FD if necessary...
*/
- vfdP->fd = BasicOpenFile(vfdP->fileName, vfdP->fileFlags,
- vfdP->fileMode);
+ vfdP->fd = BasicOpenFilePerm(vfdP->fileName, vfdP->fileFlags,
+ vfdP->fileMode);
if (vfdP->fd < 0)
{
DO_DB(elog(LOG, "re-open failed: %m"));
@@ -1293,6 +1303,16 @@ FileInvalidate(File file)
#endif
/*
+ * Open a file with PathNameOpenFilePerm() and pass PG_FILE_MODE_DEFAULT to the
+ * fileMode parameter.
+ */
+File
+PathNameOpenFile(FileName fileName, int fileFlags)
+{
+ return PathNameOpenFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT);
+}
+
+/*
* open a file in an arbitrary directory
*
* NB: if the passed pathname is relative (which it usually is),
@@ -1300,13 +1320,13 @@ FileInvalidate(File file)
* (which should always be $PGDATA when this code is running).
*/
File
-PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
+PathNameOpenFilePerm(FileName fileName, int fileFlags, int fileMode)
{
char *fnamecopy;
File file;
Vfd *vfdP;
- DO_DB(elog(LOG, "PathNameOpenFile: %s %x %o",
+ DO_DB(elog(LOG, "PathNameOpenFilePerm: %s %x %o",
fileName, fileFlags, fileMode));
/*
@@ -1324,7 +1344,7 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
/* Close excess kernel FDs. */
ReleaseLruFiles();
- vfdP->fd = BasicOpenFile(fileName, fileFlags, fileMode);
+ vfdP->fd = BasicOpenFilePerm(fileName, fileFlags, fileMode);
if (vfdP->fd < 0)
{
@@ -1461,23 +1481,21 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError)
* temp file that can be reused.
*/
file = PathNameOpenFile(tempfilepath,
- O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
- 0600);
+ O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
if (file <= 0)
{
/*
* We might need to create the tablespace's tempfile directory, if no
* one has yet done so.
*
- * Don't check for error from mkdir; it could fail if someone else
+ * Don't check for error from MakeDirectory; it could fail if someone else
* just did the same thing. If it doesn't work then we'll bomb out on
* the second create attempt, instead.
*/
- mkdir(tempdirpath, S_IRWXU);
+ MakeDirectory(tempdirpath);
file = PathNameOpenFile(tempfilepath,
- O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
- 0600);
+ O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
if (file <= 0 && rejectError)
elog(ERROR, "could not create temporary file \"%s\": %m",
tempfilepath);
@@ -2136,12 +2154,21 @@ TryAgain:
return NULL;
}
+/*
+ * Open a file with OpenTransientFilePerm() and pass PG_FILE_MODE_DEFAULT to
+ * the fileMode parameter.
+ */
+int
+OpenTransientFile(FileName fileName, int fileFlags)
+{
+ return OpenTransientFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT);
+}
/*
* Like AllocateFile, but returns an unbuffered fd like open(2)
*/
int
-OpenTransientFile(FileName fileName, int fileFlags, int fileMode)
+OpenTransientFilePerm(FileName fileName, int fileFlags, int fileMode)
{
int fd;
@@ -2158,7 +2185,7 @@ OpenTransientFile(FileName fileName, int fileFlags, int fileMode)
/* Close excess kernel FDs. */
ReleaseLruFiles();
- fd = BasicOpenFile(fileName, fileFlags, fileMode);
+ fd = BasicOpenFilePerm(fileName, fileFlags, fileMode);
if (fd >= 0)
{
@@ -3081,7 +3108,7 @@ pre_sync_fname(const char *fname, bool isdir, int elevel)
if (isdir)
return;
- fd = OpenTransientFile((char *) fname, O_RDONLY | PG_BINARY, 0);
+ fd = OpenTransientFile((char *) fname, O_RDONLY | PG_BINARY);
if (fd < 0)
{
@@ -3141,7 +3168,7 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
else
flags |= O_RDONLY;
- fd = OpenTransientFile((char *) fname, flags, 0);
+ fd = OpenTransientFile((char *) fname, flags);
/*
* Some OSs don't allow us to open directories at all (Windows returns
@@ -3213,3 +3240,22 @@ fsync_parent_path(const char *fname, int elevel)
return 0;
}
+
+/*
+ * Create a directory with MakeDirectoryPerm() and pass PG_DIR_MODE_DEFAULT to
+ * the directoryMode parameter.
+ */
+int
+MakeDirectory(const char *directoryName)
+{
+ return MakeDirectoryPerm(directoryName, PG_DIR_MODE_DEFAULT);
+}
+
+/*
+ * Create a directory with the specified mode.
+ */
+int
+MakeDirectoryPerm(const char *directoryName, int directoryMode)
+{
+ return mkdir(directoryName, directoryMode);
+}
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 1500465d31..3cf34dbc4f 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -835,7 +835,7 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
/* Create new segment or open an existing one for attach or resize. */
flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0);
- if ((fd = OpenTransientFile(name, flags, 0600)) == -1)
+ if ((fd = OpenTransientFilePerm(name, flags, S_IRUSR | S_IWUSR)) == -1)
{
if (errno != EEXIST)
ereport(elevel,
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index dfb47e7c39..f3c9d9f2d3 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -132,8 +132,8 @@ proc_exit(int code)
else
snprintf(gprofDirName, 32, "gprof/%d", (int) getpid());
- mkdir("gprof", S_IRWXU | S_IRWXG | S_IRWXO);
- mkdir(gprofDirName, S_IRWXU | S_IRWXG | S_IRWXO);
+ MakeDirectoryPerm("gprof", S_IRWXU | S_IRWXG | S_IRWXO);
+ MakeDirectoryPerm(gprofDirName, S_IRWXU | S_IRWXG | S_IRWXO);
chdir(gprofDirName);
}
#endif
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 65e0abe9ec..64a4ccf0db 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -304,7 +304,7 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
path = relpath(reln->smgr_rnode, forkNum);
- fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
{
@@ -317,7 +317,7 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
* already, even if isRedo is not set. (See also mdopen)
*/
if (isRedo || IsBootstrapProcessingMode())
- fd = PathNameOpenFile(path, O_RDWR | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
/* be sure to report the error reported by create, not open */
@@ -430,7 +430,7 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
/* truncate(2) would be easier here, but Windows hasn't got it */
int fd;
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY, 0);
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
if (fd >= 0)
{
int save_errno;
@@ -583,7 +583,7 @@ mdopen(SMgrRelation reln, ForkNumber forknum, int behavior)
path = relpath(reln->smgr_rnode, forknum);
- fd = PathNameOpenFile(path, O_RDWR | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);
if (fd < 0)
{
@@ -594,7 +594,7 @@ mdopen(SMgrRelation reln, ForkNumber forknum, int behavior)
* substitute for mdcreate() in bootstrap mode only. (See mdcreate)
*/
if (IsBootstrapProcessingMode())
- fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600);
+ fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
if (fd < 0)
{
if ((behavior & EXTENSION_RETURN_NULL) &&
@@ -1780,7 +1780,7 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
fullpath = _mdfd_segpath(reln, forknum, segno);
/* open the file */
- fd = PathNameOpenFile(fullpath, O_RDWR | PG_BINARY | oflags, 0600);
+ fd = PathNameOpenFile(fullpath, O_RDWR | PG_BINARY | oflags);
pfree(fullpath);
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index f5394dc43d..41c2ba7f97 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -644,8 +644,7 @@ load_relmap_file(bool shared)
}
/* Read data ... */
- fd = OpenTransientFile(mapfilename,
- O_RDONLY | PG_BINARY, S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(mapfilename, O_RDONLY | PG_BINARY);
if (fd < 0)
ereport(FATAL,
(errcode_for_file_access(),
@@ -745,9 +744,7 @@ write_relmap_file(bool shared, RelMapFile *newmap,
realmap = &local_map;
}
- fd = OpenTransientFile(mapfilename,
- O_WRONLY | O_CREAT | PG_BINARY,
- S_IRUSR | S_IWUSR);
+ fd = OpenTransientFile(mapfilename, O_WRONLY | O_CREAT | PG_BINARY);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bc9f09a086..e520fbace9 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7244,8 +7244,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
* truncate and reuse it.
*/
Tmpfd = BasicOpenFile(AutoConfTmpFileName,
- O_CREAT | O_RDWR | O_TRUNC,
- S_IRUSR | S_IWUSR);
+ O_CREAT | O_RDWR | O_TRUNC);
if (Tmpfd < 0)
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index faef39e78d..6a7b6121e9 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -22,7 +22,7 @@
* Use them for all file activity...
*
* File fd;
- * fd = PathNameOpenFile("foo", O_RDONLY, 0600);
+ * fd = PathNameOpenFile("foo", O_RDONLY);
*
* AllocateFile();
* FreeFile();
@@ -59,13 +59,17 @@ extern int max_files_per_process;
*/
extern int max_safe_fds;
+/* Define default file/directory modes. */
+#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR)
+#define PG_DIR_MODE_DEFAULT (S_IRWXU)
/*
* prototypes for functions in fd.c
*/
/* Operations on virtual Files --- equivalent to Unix kernel file ops */
-extern File PathNameOpenFile(FileName fileName, int fileFlags, int fileMode);
+extern File PathNameOpenFile(FileName fileName, int fileFlags);
+extern File PathNameOpenFilePerm(FileName fileName, int fileFlags, int fileMode);
extern File OpenTemporaryFile(bool interXact);
extern void FileClose(File file);
extern int FilePrefetch(File file, off_t offset, int amount, uint32 wait_event_info);
@@ -94,11 +98,17 @@ extern struct dirent *ReadDir(DIR *dir, const char *dirname);
extern int FreeDir(DIR *dir);
/* Operations to allow use of a plain kernel FD, with automatic cleanup */
-extern int OpenTransientFile(FileName fileName, int fileFlags, int fileMode);
+extern int OpenTransientFile(FileName fileName, int fileFlags);
+extern int OpenTransientFilePerm(FileName fileName, int fileFlags, int fileMode);
extern int CloseTransientFile(int fd);
/* If you've really really gotta have a plain kernel FD, use this */
-extern int BasicOpenFile(FileName fileName, int fileFlags, int fileMode);
+extern int BasicOpenFile(FileName fileName, int fileFlags);
+extern int BasicOpenFilePerm(FileName fileName, int fileFlags, int fileMode);
+
+/* Operations to make directories */
+extern int MakeDirectory(const char *directoryName);
+extern int MakeDirectoryPerm(const char *directoryName, int directoryMode);
/* Miscellaneous support routines */
extern void InitFileAccess(void);
Re: OpenFile() Permissions Refactor
On 9/13/17 10:26, David Steele wrote:
Here's a new patch based on your review. Where I had a question I made
a choice as described below:
I have committed the changes to the file APIs and a fix for the umask
save/restore issue.
The mkdir changes didn't really inspire me. Replacing mkdir() with
MakeDirectoryPerm() without any change in functionality doesn't really
improve clarity. Maybe we'll revisit that when your next patches arrive.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: OpenFile() Permissions Refactor
On 9/23/17 10:22 AM, Peter Eisentraut wrote:
On 9/13/17 10:26, David Steele wrote:
Here's a new patch based on your review. Where I had a question I made
a choice as described below:I have committed the changes to the file APIs and a fix for the umask
save/restore issue.
Thank you!
The mkdir changes didn't really inspire me. Replacing mkdir() with
MakeDirectoryPerm() without any change in functionality doesn't really
improve clarity.
OK. I had hoped removing the need to specify the mode at every call
site was functionality enough. Even so, I'm a little surprised you
didn't keep PG_DIR_MODE_DEFAULT.
Maybe we'll revisit that when your next patches arrive.
The next patch set was be this same refactor on the tools (initdb,
pg_rewind, etc), but if you think the mkdir refactor did not add enough
value then I'll rethink my plans.
I may need to present all the patches in one CF so it's clear where all
this is going: allowing group read on $PGDATA.
Thanks,
--
-David
david@pgmasters.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers