Set CLOEXEC flag when opening files and sockets #1206

ensc · 2022-12-27T15:06:44Z

Avoids leaking of file descriptors in multi threaded programs by:

setting O_CLOEXEC flag in rrd_open()
using the "e" flag for fopen() and providing a fallback implementation for environments without this flag

Signed-off-by: Enrico Scholz <[email protected]>

Although the "e" fopen() flag (atomic FD_CLOEXEC support) is scheduled for being added to the next POSIX version, it is not supported by all platforms. Check whether it is accepted and working. Because this flag can be tested at runtime only, configure.ac uses AC_RUN_IFELSE. Cross compiling fallback assumes that "e" is supported. Signed-off-by: Enrico Scholz <[email protected]>

Signed-off-by: Enrico Scholz <[email protected]>

Just define 'O_CLOEXEC' as 0; it is used always like in | f = open(..., flags | O_CLOEXEC); Signed-off-by: Enrico Scholz <[email protected]>

Signed-off-by: Enrico Scholz <[email protected]>

When fopen() does not support the "e" flag, parse the mode string and run an 'open(..., O_CLOEXEC) + fdopen()' sequence when it is set. Signed-off-by: Enrico Scholz <[email protected]>

Avoid leaking file descriptors by set the O_CLOEXEC flag. This flag is part of POSIX.1-2008 and there is implemented a fallback for systems without it. Signed-off-by: Enrico Scholz <[email protected]>

Signed-off-by: Enrico Scholz <[email protected]>

ensc · 2022-12-29T11:07:07Z

I do not know why the linux builds in the CI are failing; they succeed locally. Perhaps a problem with the github buildsystem.

oetiker · 2023-01-04T08:27:30Z

looking into the test issue ... guess this might be unrelated to your changes

codecov-commenter · 2023-01-04T10:05:25Z

Codecov Report

Merging #1206 (3f45243) into master (f0b2954) will increase coverage by 0.01%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #1206      +/-   ##
==========================================
+ Coverage   50.98%   50.99%   +0.01%     
==========================================
  Files          45       45              
  Lines       18043    18043              
==========================================
+ Hits         9199     9201       +2     
+ Misses       8844     8842       -2

Impacted Files	Coverage Δ
src/rrd_cgi.c	`0.00% <0.00%> (ø)`
src/rrd_graph.c	`51.60% <0.00%> (+0.02%)`	⬆️
src/rrd_xport.c	`58.71% <0.00%> (ø)`
src/rrd_daemon.c	`47.85% <33.33%> (ø)`
src/rrd_client.c	`62.87% <100.00%> (ø)`
src/rrd_dump.c	`63.10% <100.00%> (ø)`
src/rrd_open.c	`59.35% <100.00%> (ø)`
src/rrd_modify.c	`68.96% <0.00%> (+0.14%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

c72578 · 2023-04-20T16:35:15Z

src/compat-cloexec.c

+#include <stdlib.h>
+#include <string.h>
+
+#ifdef _POSIX_C_SOURCE


@ensc Could you please check:

#ifdef _POSIX_C_SOURCE # include <fcntl.h>

In msys2 mingw-w64 builds we see these warnings e.g. [1]:

compat-cloexec.c:11: warning: "O_CREAT" redefined 11 | # define O_CREAT 0 | In file included from ./rrd_config_bottom.h:64, from ./rrd_config.h:634, from compat-cloexec.h:4, from compat-cloexec.c:1: D:/a/_temp/msys64/mingw64/include/fcntl.h:40: note: this is the location of the previous definition 40 | #define O_CREAT _O_CREAT | compat-cloexec.c:12: warning: "O_WRONLY" redefined 12 | # define O_WRONLY 0 | D:/a/_temp/msys64/mingw64/include/fcntl.h:37: note: this is the location of the previous definition 37 | #define O_WRONLY _O_WRONLY | compat-cloexec.c:13: warning: "O_RDONLY" redefined 13 | # define O_RDONLY 0 | D:/a/_temp/msys64/mingw64/include/fcntl.h:36: note: this is the location of the previous definition 36 | #define O_RDONLY _O_RDONLY | compat-cloexec.c:14: warning: "O_APPEND" redefined 14 | # define O_APPEND 0 | D:/a/_temp/msys64/mingw64/include/fcntl.h:39: note: this is the location of the previous definition 39 | #define O_APPEND _O_APPEND | compat-cloexec.c:15: warning: "O_RDWR" redefined 15 | # define O_RDWR 0 | D:/a/_temp/msys64/mingw64/include/fcntl.h:38: note: this is the location of the previous definition 38 | #define O_RDWR _O_RDWR |

configure says:

checking for fcntl.h... yes

So, HAVE_FCNTL_H is defined.

[1] https://github.com/oetiker/rrdtool-1.x/actions/runs/4740218558/jobs/8415805010#step:5:901

c72578 · 2023-04-20T16:41:55Z

tests/test_compat-cloexec.c

+#include <stdlib.h>
+#include <stdbool.h>
+
+#ifdef _POSIX_C_SOURCE


@ensc Warnings under msys2 mingw-w64 as in compat-cloexec.c:

CC test_compat-cloexec.o test_compat-cloexec.c:10: warning: "O_RDONLY" redefined 10 | # define O_RDONLY 0 | In file included from ../src/rrd_config_bottom.h:64, from ../src/rrd_config.h:634, from ../src/compat-cloexec.h:4, from test_compat-cloexec.c:1: C:/msys64/mingw64/include/fcntl.h:36: note: this is the location of the previous definition 36 | #define O_RDONLY _O_RDONLY | test_compat-cloexec.c:11: warning: "O_RDWR" redefined 11 | # define O_RDWR 0 | C:/msys64/mingw64/include/fcntl.h:38: note: this is the location of the previous definition 38 | #define O_RDWR _O_RDWR | test_compat-cloexec.c:12: warning: "O_APPEND" redefined 12 | # define O_APPEND 0 | C:/msys64/mingw64/include/fcntl.h:39: note: this is the location of the previous definition 39 | #define O_APPEND _O_APPEND | CC compat-cloexec.o ../src/compat-cloexec.c:11: warning: "O_CREAT" redefined 11 | # define O_CREAT 0 | In file included from ../src/rrd_config_bottom.h:64, from ../src/rrd_config.h:634, from ../src/compat-cloexec.h:4, from ../src/compat-cloexec.c:1: C:/msys64/mingw64/include/fcntl.h:40: note: this is the location of the previous definition 40 | #define O_CREAT _O_CREAT | ../src/compat-cloexec.c:12: warning: "O_WRONLY" redefined 12 | # define O_WRONLY 0 | C:/msys64/mingw64/include/fcntl.h:37: note: this is the location of the previous definition 37 | #define O_WRONLY _O_WRONLY | ../src/compat-cloexec.c:13: warning: "O_RDONLY" redefined 13 | # define O_RDONLY 0 | C:/msys64/mingw64/include/fcntl.h:36: note: this is the location of the previous definition 36 | #define O_RDONLY _O_RDONLY | ../src/compat-cloexec.c:14: warning: "O_APPEND" redefined 14 | # define O_APPEND 0 | C:/msys64/mingw64/include/fcntl.h:39: note: this is the location of the previous definition 39 | #define O_APPEND _O_APPEND | ../src/compat-cloexec.c:15: warning: "O_RDWR" redefined 15 | # define O_RDWR 0 | C:/msys64/mingw64/include/fcntl.h:38: note: this is the location of the previous definition 38 | #define O_RDWR _O_RDWR | CCLD compat-cloexec.exe

c72578 · 2023-04-20T16:43:58Z

tests/test_compat-cloexec.c

+	FILE *f;
+
+	f = _rrd_fopen("/dev/null", "r");
+	check_file(f, O_RDONLY, false, __LINE__);


The test fails under msys2 mingw-w64:

test_compat-cloexec.c:59 fopen() failed FAIL compat-cloexec.exe (exit status: 3)

should we skip it on windows ?

configure: check for O_CLOEXEC

0d2f282

Signed-off-by: Enrico Scholz <[email protected]>

ensc changed the title ~~Feature cloexec~~ Set CLOEXEC flag when opening files and sockest Dec 28, 2022

ensc changed the title ~~Set CLOEXEC flag when opening files and sockest~~ Set CLOEXEC flag when opening files and sockets Dec 28, 2022

ensc added 10 commits December 28, 2022 15:10

configure: check for SOCK_CLOEXEC

ec61bde

Signed-off-by: Enrico Scholz <[email protected]>

compat-cloexec: initial checkin

587e14c

Signed-off-by: Enrico Scholz <[email protected]>

compat-cloexec: implement missing HAVE_DECL_O_CLOEXEC case

8fde730

Just define 'O_CLOEXEC' as 0; it is used always like in | f = open(..., flags | O_CLOEXEC); Signed-off-by: Enrico Scholz <[email protected]>

compat-cloexec: implement missing HAVE_DECL_SOCK_CLOEXEC case

c9a7fa9

Signed-off-by: Enrico Scholz <[email protected]>

compat-cloexec: implement missing RD_HAVE_WORKING_FOPEN_E case

6b4ae55

When fopen() does not support the "e" flag, parse the mode string and run an 'open(..., O_CLOEXEC) + fdopen()' sequence when it is set. Signed-off-by: Enrico Scholz <[email protected]>

rrd_open: open file with O_CLOEXEC

f979e91

Avoid leaking file descriptors by set the O_CLOEXEC flag. This flag is part of POSIX.1-2008 and there is implemented a fallback for systems without it. Signed-off-by: Enrico Scholz <[email protected]>

open sockets with SOCK_CLOEXEC

35e9c68

Signed-off-by: Enrico Scholz <[email protected]>

set "e" flag with fopen()

9e6db60

Signed-off-by: Enrico Scholz <[email protected]>

tests: add test for _rrd_fopen()

5a117bb

Signed-off-by: Enrico Scholz <[email protected]>

ensc force-pushed the feature-cloexec branch from 5dd45a2 to 5a117bb Compare December 29, 2022 11:05

ensc marked this pull request as ready for review December 29, 2022 11:06

Merge branch 'master' into feature-cloexec

3c0833a

Update CHANGES

3f45243

oetiker approved these changes Jan 4, 2023

View reviewed changes

oetiker merged commit 6d158d1 into oetiker:master Jan 6, 2023

c72578 reviewed Apr 20, 2023

View reviewed changes

c72578 mentioned this pull request Jul 31, 2024

error: use of undeclared identifier 'SOCK_CLOEXEC' for macos builds #1261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set CLOEXEC flag when opening files and sockets #1206

Set CLOEXEC flag when opening files and sockets #1206

ensc commented Dec 27, 2022 •

edited

Loading

ensc commented Dec 29, 2022

oetiker commented Jan 4, 2023

codecov-commenter commented Jan 4, 2023 •

edited

Loading

c72578 Apr 20, 2023

c72578 Apr 20, 2023

c72578 Apr 20, 2023

oetiker Apr 21, 2023

Set CLOEXEC flag when opening files and sockets #1206

Set CLOEXEC flag when opening files and sockets #1206

Conversation

ensc commented Dec 27, 2022 • edited Loading

ensc commented Dec 29, 2022

oetiker commented Jan 4, 2023

codecov-commenter commented Jan 4, 2023 • edited Loading

Codecov Report

c72578 Apr 20, 2023

Choose a reason for hiding this comment

c72578 Apr 20, 2023

Choose a reason for hiding this comment

c72578 Apr 20, 2023

Choose a reason for hiding this comment

oetiker Apr 21, 2023

Choose a reason for hiding this comment

ensc commented Dec 27, 2022 •

edited

Loading

codecov-commenter commented Jan 4, 2023 •

edited

Loading