Message-ID: <68480868.5d0a0220.1e214d.68a6@mx.google.com>
Sender: Michael Banck <mbanck@gmail.com>
Date: Tue, 10 Jun 2025 12:26:48 +0200
From: Michael Banck <mbanck@gmx.net>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Safeguards against incorrect fd flags for fsync()
References: <20191009062640.GB21379@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ZPt4rx8FFjLCG7dd"
Content-Disposition: inline
In-Reply-To: <20191009062640.GB21379@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/68480868.5d0a0220.1e214d.68a6%40mx.google.com>
Precedence: bulk


--ZPt4rx8FFjLCG7dd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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
 

--ZPt4rx8FFjLCG7dd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Make-safeguard-against-incorrect-fd-flags-for-fsync-.patch"

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


--ZPt4rx8FFjLCG7dd--