postgres.git / summary / log / commit / refs

commit    12198239c0a5122e29619d50f76f89adc5bc7ade
Author:   Michael Paquier <michael@paquier.xyz>
Date:     Tue Nov 26 04:32:52 2019 +0000
First released in: 13.0

    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.GB21379@paquier.xyz


 src/backend/storage/file/fd.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f18bcb2f78e..f4ab19f8b76 100644
--- 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)


[parent: 080313f8296f]