Add safeguards for pg_fsync() called with incorrectly-opened fds

On some platforms, fsync() returns EBADFD when opening a file descriptor
with O_RDONLY (read-only), leading ultimately now to a PANIC to prevent
data corruption.

This commit adds a new sanity check in pg_fsync() based on fcntl() to
make sure that we don't repeat again mistakes with incorrectly-set file
descriptors so as problems are detected at an early stage.  Without
that, such errors could only be detected after running Postgres on a
specific supported platform for the culprit code path, which could take
some time before being found.  b8e19b93 was a fix for such a problem,
which got undetected for more than 5 years, and a586cc4b fixed another
similar issue.

Note that the new check added works as well when fsync=off is
configured, so as all regression tests would detect problems as long as
assertions are enabled.  fcntl() being not available on Windows, the
new checks do not happen there.

Author: Michael Paquier
Reviewed-by: Mark Dilger
Discussion: https://postgr.es/m/20191009062640[email protected]

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f18bcb2f78e2b70f326fe621eddd8bd62a3f71db..f4ab19f8b7638563d0af74977cbaf83300cae5b3 100644 (file)
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -329,6 +329,44 @@ static int fsync_parent_path(const char *fname, int elevel);
 int
 pg_fsync(int fd)
 {
+#if !defined(WIN32) && defined(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 without
+        * write permissions (O_RDONLY).
+        *
+        * Ignore any fstat errors and let the follow-up fsync() do its work.
+        * Doing this sanity check here counts for the case where fsync() is
+        * disabled.
+        */
+       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);
+       }
+       errno = 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)