From: Mark Dilger <hornschnorter@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Safeguards against incorrect fd flags for fsync()
Date: Thu, 7 Nov 2019 13:57:57 -0800
Message-ID: <82db297a-74f7-ca23-545b-3c531d692e4d@gmail.com> (raw)
In-Reply-To: <20191009062640.GB21379@paquier.xyz>
References: <20191009062640.GB21379@paquier.xyz>
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-196fc97cc05e141c@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.cindex 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)
Attachments:
[text/plain] fsync-safeguards.patch.2 (1.6K, ../82db297a-74f7-ca23-545b-3c531d692e4d@gmail.com/2-fsync-safeguards.patch.2)
download | inline diff:diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.cindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hornschnorter@gmail.com, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: Safeguards against incorrect fd flags for fsync()
In-Reply-To: <82db297a-74f7-ca23-545b-3c531d692e4d@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox