[Michael Banck <mbanck@gmx.net>]

Hi,

On Wed, Oct 09, 2019 at 03:26:40PM +0900, Michael Paquier wrote:
> 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?

Well O_RDONLY might historically be 0 almost everywhere, but it is
defined to 1 on the GNU system [1]:

|#define      O_RDONLY        0x0001 /* Open read-only.  */

So there, the comparison with 0 does not work and initdb (at least)
fails on assert-enabled builds:

|running bootstrap script ... TRAP: FailedAssertion("(desc_flags & (O_RDWR | O_WRONLY)) == 0", File: "fd.c", Line: 395, PID: 4560)

TTBOMK, POSIX does not mandate that O_RDONLY be 0, so I think this check
is overly zealous. The better way might be to mask the flags with
O_ACCMODE and then just check what you want, like in the attached.

Thoughts?


Michael

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/mach/hurd/bits/fcntl.h;hb=HEAD#l39
 

Attachments:

  [text/x-diff] 0001-Make-safeguard-against-incorrect-fd-flags-for-fsync-.patch (1.2K, ../68480868.5d0a0220.1e214d.68a6@mx.google.com/2-0001-Make-safeguard-against-incorrect-fd-flags-for-fsync-.patch)
  download | inline diff:
From 16431e7b1aac9600fac285e77f39bb628a0dea14 Mon Sep 17 00:00:00 2001
From: Michael Banck <mbanck@debian.org>
Date: Tue, 10 Jun 2025 12:23:17 +0200
Subject: [PATCH] Make safeguard against incorrect fd flags for fsync more
 portable.

The current code assumed that O_RDONLY is defined as 0, but this is not
universally the case. To fix this, mask the flags with O_ACCMODE and
assert that they are O_RDONLY for directories or not O_RDONLY for files.
---
 src/backend/storage/file/fd.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 0e8299dd556..cd1e661974a 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -411,14 +411,12 @@ pg_fsync(int fd)
 	{
 		int			desc_flags = fcntl(fd, F_GETFL);
 
-		/*
-		 * O_RDONLY is historically 0, so just make sure that for directories
-		 * no write flags are used.
-		 */
+		desc_flags &= O_ACCMODE;
+
 		if (S_ISDIR(st.st_mode))
-			Assert((desc_flags & (O_RDWR | O_WRONLY)) == 0);
+			Assert(desc_flags == O_RDONLY);
 		else
-			Assert((desc_flags & (O_RDWR | O_WRONLY)) != 0);
+			Assert(desc_flags != O_RDONLY);
 	}
 	errno = 0;
 #endif
-- 
2.39.5