On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote: > 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.
Thanks for the suggestions, sounds fine to me. > 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 */ The check order in the former patch is consistent with what's done at the top of fsync_fname_ext(), still I can see your point. So let's do as you suggest. > 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. The only possible error which could be expected here would be a ENOENT so we could filter after that, but fsync() would most likely complain about that so it sounds better to let it do its work with its own logging, which would be more helpful for the user, if of course we have fsync=on in postgresql.conf. > Attached is a revised version of the patch. Perhaps you can check what I've > done and tell me if I've broken it. Thanks for the review. I was wondering why I did not do that as well for file_utils.c, just to find out that fsync_fname() is the only entry point in file_utils.c. Anyway, the patch had a problem regarding fcntl() which is not available on Windows (see for example pg_set_noblock in noblock.c). Performing the sanity check will allow to catch any problems for all platforms we support, so let's just skip it for Windows. For this reason it is better as well to update errno to 0 after the fstat() call. Who knows... Attached is an updated version, with your changes included. How does that look? -- Michael
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f18bcb2f78..9849bba9bd 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 was opened
+ * 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)
signature.asc
Description: PGP signature
