From: Enrico Scholz Date: Fri, 6 Jan 2023 11:02:12 +0000 (+0100) Subject: Set CLOEXEC flag when opening files and sockets (#1206) X-Git-Tag: v1.9.0~19 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6d158d14fa8843152608696b69d1ecf38bd012fe;p=thirdparty%2Frrdtool-1.x.git Set CLOEXEC flag when opening files and sockets (#1206) * configure: check for O_CLOEXEC * configure: check for SOCK_CLOEXEC * configure: check whether fopen() supports the "e" flag 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. * compat-cloexec: initial checkin * compat-cloexec: implement missing HAVE_DECL_O_CLOEXEC case Just define 'O_CLOEXEC' as 0; it is used always like in | f = open(..., flags | O_CLOEXEC); * compat-cloexec: implement missing HAVE_DECL_SOCK_CLOEXEC case * compat-cloexec: implement missing RD_HAVE_WORKING_FOPEN_E case When fopen() does not support the "e" flag, parse the mode string and run an 'open(..., O_CLOEXEC) + fdopen()' sequence when it is set. * rrd_open: open file with O_CLOEXEC 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. * open sockets with SOCK_CLOEXEC * set "e" flag with fopen() * tests: add test for _rrd_fopen() Signed-off-by: Enrico Scholz --- diff --git a/CHANGES b/CHANGES index dc0fd017..5cfc048f 100644 --- a/CHANGES +++ b/CHANGES @@ -11,14 +11,13 @@ Bugfixes * Fix BUILD_DATE in rrdtool help output * acinclude.m4: Include when using exit * rrdtool-release: Create NUMVERS from VERSION file - +* Avoids leaking of file descriptors in multi threaded programs by @ensc Features -------- * Remove autogenerated files from git repo (configure, Makefile.in, conftools, rrd_config.h.in) - RRDtool 1.8.0 - 2022-03-13 ========================== diff --git a/configure.ac b/configure.ac index 11029e4f..be8b4d6e 100644 --- a/configure.ac +++ b/configure.ac @@ -509,6 +509,46 @@ AC_CHECK_SIZEOF([time_t]) AC_CHECK_SIZEOF([long int]) +AC_CHECK_DECLS([O_CLOEXEC], [], [], [#include ]) +AC_CHECK_DECLS([SOCK_CLOEXEC], [], [], [#include ]) + +AC_CACHE_CHECK([whether fopen() supports the "e" flag],[rd_cv_fopen_e],[ + AC_RUN_IFELSE([ + AC_LANG_PROGRAM([ + #include + #include + #include + ], [ + FILE *f = fopen("/dev/null", "re"); + int fd; + long flags; + + if (!f) + /* "e" causes fopen() to fail */ + return 1; + fd = fileno(f); + if (fd < 0) + return 2; + + flags = fcntl(fileno(f), F_GETFD); + if (flags < 0 || (flags & FD_CLOEXEC) == 0) + /* "e" is accepted but has no effect */ + return 3; + ]) + ], + [rd_cv_fopen_e=yes], + [rd_cv_fopen_e=no],[ + dnl cross-compiling; assume yes + rd_cv_fopen_e=yes + ]) +]) + +AS_IF([test x"$rd_cv_fopen_e" = xyes],[ + AC_DEFINE([RRD_HAVE_WORKING_FOPEN_E], [1], [indicates whether fopen(..., "e") is working]) +]) + +AM_CONDITIONAL([NEED_COMPAT_CLOEXEC],[test x"$rd_cv_fopen_e" != xyes]) + CONFIGURE_PART(Find 3rd-Party Libraries) have_libdbi=no diff --git a/src/Makefile.am b/src/Makefile.am index 069f0548..b116b971 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -71,6 +71,7 @@ noinst_HEADERS = \ unused.h \ gettext.h \ mutex.h \ + compat-cloexec.h \ rrd_strtod.h \ rrd_snprintf.h \ rrd_parsetime.h \ @@ -97,6 +98,10 @@ RRD_C_FILES += ../win32/win32-glob.c strftime.c noinst_HEADERS += ../win32/win32-glob.h strftime.h endif +if NEED_COMPAT_CLOEXEC +UPD_C_FILES += compat-cloexec.c +endif + noinst_LTLIBRARIES = librrdupd.la lib_LTLIBRARIES = librrd.la diff --git a/src/compat-cloexec.c b/src/compat-cloexec.c new file mode 100644 index 00000000..6931fa7e --- /dev/null +++ b/src/compat-cloexec.c @@ -0,0 +1,101 @@ +#include "compat-cloexec.h" + +#include +#include +#include + +#ifdef _POSIX_C_SOURCE +# include +# include +#else +# define O_CREAT 0 +# define O_WRONLY 0 +# define O_RDONLY 0 +# define O_APPEND 0 +# define O_RDWR 0 +#endif + +inline static bool have_decl_o_cloexec(void) +{ +#ifdef HAVE_DECL_O_CLOEXEC + return true; +#else + return false; +#endif +} + +FILE *_rrd_fopen(const char *restrict pathname, const char *restrict mode_raw) +{ + char mode[20]; + const char *in = mode_raw; + char *out = mode; + int flags = 0; + int rw_flags = 0; + bool is_cloexec = false; + + /* We are the only caller and never use mode strings with more than 20 + chars... But just to be sure... */ + if (strlen(mode_raw) >= sizeof mode) + abort(); + + /* parse the mode string and strip away the 'e' flag */ + while (*in) { + char c = *in++; + + switch (c) { + case 'w': + flags |= O_CREAT; + rw_flags = O_WRONLY; + break; + case 'r': + rw_flags = O_RDONLY; + break; + case 'a': + flags |= O_CREAT | O_APPEND; + rw_flags = O_WRONLY; + break; + case '+': + rw_flags = O_RDWR; + break; + case 'e': + is_cloexec = true; + /* continue loop and do not copy mode char */ + continue; + case 'b': + break; + default: + /* we are the only caller and should not set any + unknown flag */ + abort(); + } + + *out++ = c; + } + + *out = '\0'; + +#ifndef _POSIX_C_SOURCE + (void)flags; + (void)rw_flags; + (void)is_cloexec; + /* TODO: do we have to care about O_CLOEXEC behavior on non-POSIX + systems? */ +#else + if (have_decl_o_cloexec() && is_cloexec) { + int fd; + FILE *res; + + fd = open(pathname, flags | rw_flags | O_CLOEXEC, 0666); + if (fd < 0) + return NULL; + + res = fdopen(fd, mode); + if (!res) + close(fd); + + return res; + } +#endif + + return fopen(pathname, mode); +} diff --git a/src/compat-cloexec.h b/src/compat-cloexec.h new file mode 100644 index 00000000..0d4d2b6e --- /dev/null +++ b/src/compat-cloexec.h @@ -0,0 +1,30 @@ +#ifndef H_RRDTOOL_SRC_COMPAT_CLOEXEC_H +#define H_RRDTOOL_SRC_COMPAT_CLOEXEC_H + +#include + +#ifndef HAVE_DECL_O_CLOEXEC +# define O_CLOEXEC 0 +#endif + +#ifndef HAVE_DECL_SOCK_CLOEXEC +# define SOCK_CLOEXEC 0 +#endif + +#include + +FILE *_rrd_fopen(const char *restrict pathname, const char *restrict mode_raw); + +#ifdef RRD_HAVE_WORKING_FOPEN_E +# define rrd_fopen(_pathname, _mode) fopen(_pathname, _mode) +#else + +inline static +FILE *rrd_fopen(const char *restrict pathname, const char *restrict mode) +{ + return _rrd_fopen(pathname, mode); +} + +#endif /* RD_HAVE_WORKING_FOPEN_E */ + +#endif /* H_RRDTOOL_SRC_COMPAT_CLOEXEC_H */ diff --git a/src/rrd_cgi.c b/src/rrd_cgi.c index db293e3c..b82c310d 100644 --- a/src/rrd_cgi.c +++ b/src/rrd_cgi.c @@ -9,6 +9,8 @@ #include #endif +#include "compat-cloexec.h" + #define MEMBLK 1024 /*#define DEBUG_PARSER #define DEBUG_VARS*/ @@ -381,7 +383,7 @@ static int readfile( if ((strcmp("-", file_name) == 0)) { input = stdin; } else { - if ((input = fopen(file_name, "rb")) == NULL) { + if ((input = rrd_fopen(file_name, "rbe")) == NULL) { rrd_set_error("opening '%s': %s", file_name, rrd_strerror(errno)); return (-1); } diff --git a/src/rrd_client.c b/src/rrd_client.c index faf462ae..51ba91d6 100644 --- a/src/rrd_client.c +++ b/src/rrd_client.c @@ -66,6 +66,8 @@ #include #include +#include "compat-cloexec.h" + struct rrdc_response_s { int status; char *message; @@ -793,7 +795,7 @@ static int connect_unix( assert(path != NULL); assert(client->sd == -1); - client->sd = socket(PF_UNIX, SOCK_STREAM, /* protocol = */ 0); + client->sd = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, /* protocol = */ 0); if (client->sd < 0) { status = errno; return (status); diff --git a/src/rrd_daemon.c b/src/rrd_daemon.c index d51a8f3d..0bc296bb 100644 --- a/src/rrd_daemon.c +++ b/src/rrd_daemon.c @@ -111,6 +111,8 @@ #endif /* HAVE_LIBWRAP */ #include "rrd_strtod.h" +#include "compat-cloexec.h" + #include /* }}} */ @@ -3478,7 +3480,7 @@ static int journal_replay( } } - fh = fopen(file, "r"); + fh = rrd_fopen(file, "re"); if (fh == NULL) { if (errno != ENOENT) RRDD_LOG(LOG_ERR, @@ -3798,7 +3800,7 @@ static int open_listen_socket_unix( listen_fds = temp; memcpy(listen_fds + listen_fds_num, sock, sizeof(listen_fds[0])); - fd = socket(PF_UNIX, SOCK_STREAM, /* protocol = */ 0); + fd = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, /* protocol = */ 0); if (fd < 0) { fprintf(stderr, "rrdcached: unix socket(2) failed: %s\n", rrd_strerror(errno)); @@ -4819,7 +4821,7 @@ static int read_options( { if (log_fh) fclose(log_fh); - log_fh = fopen(options.optarg, "a"); + log_fh = rrd_fopen(options.optarg, "ae"); if (!log_fh) { fprintf(stderr, "Failed to open log file '%s': %s\n", options.optarg, rrd_strerror(errno)); diff --git a/src/rrd_dump.c b/src/rrd_dump.c index c61d6457..26444294 100644 --- a/src/rrd_dump.c +++ b/src/rrd_dump.c @@ -46,6 +46,8 @@ #include "rrd_client.h" #include "rrd_snprintf.h" +#include "compat-cloexec.h" + #if !(defined(NETWARE) || defined(_WIN32)) extern char *tzname[2]; @@ -480,7 +482,7 @@ int rrd_dump_opt_r( out_file = NULL; if (outname) { - if (!(out_file = fopen(outname, "w"))) { + if (!(out_file = rrd_fopen(outname, "we"))) { return (-1); } } else { diff --git a/src/rrd_graph.c b/src/rrd_graph.c index b9aa3e2a..1e5ca94c 100644 --- a/src/rrd_graph.c +++ b/src/rrd_graph.c @@ -10,6 +10,7 @@ #include "rrd_strtod.h" #include "rrd_tool.h" +#include "compat-cloexec.h" /* MinGW and MinGW-w64 use the format codes from msvcrt.dll, * which does not support e.g. %F, %T or %V. Here we need %V for "Week %V". @@ -3471,7 +3472,7 @@ int lazy_check( change here ... */ if (time(NULL) - imgstat.st_mtime > (im->end - im->start) / im->xsize) return 0; - if ((fd = fopen(im->graphfile, "rb")) == NULL) + if ((fd = rrd_fopen(im->graphfile, "rbe")) == NULL) return 0; /* the file does not exist */ switch (im->imgformat) { case IF_PNG: diff --git a/src/rrd_open.c b/src/rrd_open.c index ed4939a6..c326568a 100644 --- a/src/rrd_open.c +++ b/src/rrd_open.c @@ -21,6 +21,7 @@ #endif /* WIN32 */ #include "rrd_tool.h" +#include "compat-cloexec.h" #include "unused.h" #ifdef HAVE_BROKEN_MS_ASYNC @@ -319,7 +320,7 @@ rrd_file_t *rrd_open( goto out_free; } #else - if ((rrd_simple_file->fd = open(file_name, flags, 0666)) < 0) { + if ((rrd_simple_file->fd = open(file_name, flags | O_CLOEXEC, 0666)) < 0) { rrd_set_error("opening '%s': %s", file_name, rrd_strerror(errno)); goto out_free; } diff --git a/src/rrd_xport.c b/src/rrd_xport.c index b8ca5b62..d15d33dd 100644 --- a/src/rrd_xport.c +++ b/src/rrd_xport.c @@ -19,6 +19,7 @@ #endif #include "rrd_snprintf.h" +#include "compat-cloexec.h" static int rrd_xport_fn( image_desc_t *, @@ -429,7 +430,7 @@ int rrd_graph_xport( /* if we write a file, then open it */ if (im->graphfile) { - buffer.file = fopen(im->graphfile, "w"); + buffer.file = rrd_fopen(im->graphfile, "we"); } /* do the data processing */ diff --git a/tests/.gitignore b/tests/.gitignore index e434406b..8a0dd030 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -4,3 +4,4 @@ /modify5-testa*-mod.dump* /*.log /*.trs +/compat-cloexec diff --git a/tests/Makefile.am b/tests/Makefile.am index 5d1b8422..2106fa98 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,6 +1,7 @@ TESTS = modify1 modify2 modify3 modify4 modify5 \ tune1 tune2 graph1 graph2 rpn1 rpn2 \ rrdcreate \ + compat-cloexec \ dump-restore \ create-with-source-1 create-with-source-2 create-with-source-3 \ create-with-source-4 create-with-source-and-mapping-1 \ @@ -32,3 +33,11 @@ CLEANFILES = *.rrd \ modify5-testa1-mod.dump modify5-testa2-mod.dump \ modify5-testa1-mod.dump.tmp modify5-testa2-mod.dump.tmp \ rpn1.out rpn1.output.out + +check_PROGRAMS = \ + compat-cloexec + +compat_cloexec_SOURCES = \ + test_compat-cloexec.c \ + ${top_srcdir}/src/compat-cloexec.c \ + ${top_srcdir}/src/compat-cloexec.h diff --git a/tests/test_compat-cloexec.c b/tests/test_compat-cloexec.c new file mode 100644 index 00000000..297c2686 --- /dev/null +++ b/tests/test_compat-cloexec.c @@ -0,0 +1,69 @@ +#include + +#include +#include +#include + +#ifdef _POSIX_C_SOURCE +# include +#else +# define O_RDONLY 0 +# define O_RDWR 0 +# define O_APPEND 0 +#endif + +static void fail(char *msg, int line) +{ + fprintf(stderr, "%s:%u %s\n", __FILE__, line, msg); + abort(); +} + +static void check_file(FILE *f, int exp_flags, bool is_cloexec, int line) +{ + int flags; + int fd; + + if (!f) + fail("fopen() failed", line); + +#ifndef _POSIX_C_SOURCE + (void)flags; + (void)fd; + (void)exp_flags; + (void)is_cloexec; +#else + fd = fileno(f); + if (fd < 0) + fail("failed to get fd", line); + + flags = fcntl(fd, F_GETFD); + + if (O_CLOEXEC != 0) { + if (is_cloexec != (((flags & FD_CLOEXEC) != 0))) + fail("O_CLOEXEC mismatch", line); + } + + flags = fcntl(fd, F_GETFL); + flags &= (O_RDONLY | O_WRONLY | O_RDWR | O_APPEND); + if (flags != exp_flags) + fail("flag mismatch", line); +#endif + + fclose(f); +} + +int main(void) { + FILE *f; + + f = _rrd_fopen("/dev/null", "r"); + check_file(f, O_RDONLY, false, __LINE__); + + f = _rrd_fopen("/dev/null", "re"); + check_file(f, O_RDONLY, true, __LINE__); + + f = _rrd_fopen("/dev/null", "w+be"); + check_file(f, O_RDWR, true, __LINE__); + + f = _rrd_fopen("/dev/null", "a+be"); + check_file(f, O_RDWR | O_APPEND, true, __LINE__); +} diff --git a/win32/Makefile.msc b/win32/Makefile.msc index c958ef3e..2a998f30 100644 --- a/win32/Makefile.msc +++ b/win32/Makefile.msc @@ -38,6 +38,7 @@ THIRD_PARTY_LIB = /LIBPATH:$(ARCH_PATH)\lib \ Ws2_32.lib zdll.lib gthread-2.0.lib RRD_LIB_OBJ_LIST = \ + $(TOP)/src/compat-cloexec.obj \ $(TOP)/src/hash_32.obj \ $(TOP)/src/mkstemp.obj \ $(TOP)/src/mutex.obj \ diff --git a/win32/Makefile_vcpkg.msc b/win32/Makefile_vcpkg.msc index 065420b9..b3fa78d2 100644 --- a/win32/Makefile_vcpkg.msc +++ b/win32/Makefile_vcpkg.msc @@ -39,6 +39,7 @@ THIRD_PARTY_LIB = /LIBPATH:$(ARCH_PATH)\lib \ Ws2_32.lib zlib.lib RRD_LIB_OBJ_LIST = \ + $(TOP)/src/compat-cloexec.obj \ $(TOP)/src/hash_32.obj \ $(TOP)/src/mkstemp.obj \ $(TOP)/src/mutex.obj \ diff --git a/win32/librrd-8.def b/win32/librrd-8.def index f39fb98c..7f7eaed3 100644 --- a/win32/librrd-8.def +++ b/win32/librrd-8.def @@ -95,3 +95,4 @@ rrdc_ping rrdc_stats_free rrdc_stats_get rrdc_update +_rrd_fopen diff --git a/win32/librrd-8.vcxproj b/win32/librrd-8.vcxproj index 67fdc2db..e3a600b6 100644 --- a/win32/librrd-8.vcxproj +++ b/win32/librrd-8.vcxproj @@ -384,6 +384,7 @@ +