On 10/8/19 11:26 PM, Michael Paquier wrote:
Hi all,
After the set of issues discussed here, it seems to me that it would
be a good thing to have some safeguards against incorrect flags when
opening a fd which would be used for fsync():
https://www.postgresql.org/message-id/16039-196fc97cc05e1...@postgresql.org
Attached is a patch aimed at doing that. Historically O_RDONLY is 0,
so when looking at a directory we just need to make sure that no write
flags are used. For files, that's the contrary, a write flag has to
be used.
Thoughts or better ideas?
The code and comments don't clearly indicate what you have said in the
email, that you are verifying directories are opened read-only and files
are opened either read-write or write-only. I'd recommend changing the
comments a bit to make that clearer.
I would also rearrange the code a little, as it is slightly clearer to read:
if (x)
/* directory stuff */
else
/* file stuff */
than as you have it:
if (!x)
/* file stuff */
else
/* directory stuff */
because it takes slightly less time for somebody reading the code when
they don't have to think about the negation of x.
I'm a little uncertain about ignoring fstat errors as you do, but left
that part of the logic alone. I understand that any fstat error will
likely be immediately followed by another error when the fsync is
attempted, but relying on that seems vaguely similar to the security
vulnerability of checking permissions and then opening a file as two
separate operations. Not sure the analogy actually holds for fstat
before fsync, though.
Attached is a revised version of the patch. Perhaps you can check what
I've done and tell me if I've broken it.
--
Mark Dilger
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index fe2bb8f859..1cddd590b8 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -330,6 +330,41 @@ static int fsync_parent_path(const char *fname, int
elevel);
int
pg_fsync(int fd)
{
+#ifdef USE_ASSERT_CHECKING
+ struct stat st;
+
+ /*
+ * Some operating system implementations of fsync have requirements
about
+ * the file access modes that were used when their file descriptor
argument
+ * was opened, and these requirements differ depending on whether the
file
+ * descriptor is for a directory.
+ *
+ * For any file descriptor that may eventually be handed to fsync, we
+ * should have opened it with access modes that are compatible with
fsync
+ * on all supported systems, otherwise the code may not be portable,
even
+ * if it runs ok on the current system.
+ *
+ * We assert here that a descriptor for a file was opened with write
+ * permissions (either O_RDWR or O_WRONLY) and for a directory was
opened
+ * without write permissions (O_RDONLY).
+ *
+ * Ignore any fstat errors.
+ */
+ if (fstat(fd, &st) == 0)
+ {
+ int desc_flags = fcntl(fd, F_GETFL);
+
+ /*
+ * O_RDONLY is historically 0, so just make sure that for
+ * directories no write flags are used.
+ */
+ if (S_ISDIR(st.st_mode))
+ Assert((desc_flags & (O_RDWR | O_WRONLY)) == 0);
+ else
+ Assert((desc_flags & (O_RDWR | O_WRONLY)) != 0);
+ }
+#endif
+
/* #if is to skip the sync_method test if there's no need for it */
#if defined(HAVE_FSYNC_WRITETHROUGH) && !defined(FSYNC_WRITETHROUGH_IS_FSYNC)
if (sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH)
