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/[email protected]
>
> 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
>From 16431e7b1aac9600fac285e77f39bb628a0dea14 Mon Sep 17 00:00:00 2001
From: Michael Banck <[email protected]>
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
