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 <enrico.scholz@ensc.de>

diff --git a/CHANGES b/CHANGES
index dc0fd017bc37f97035a88aea465f4dcf41d4d12e..5cfc048fb5f1a88d512cede406f1031c325384f8 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -11,14 +11,13 @@ Bugfixes
 * Fix BUILD_DATE in rrdtool help output <c72578>
 * acinclude.m4: Include <stdlib.h> when using exit <ryandesign>
 * rrdtool-release: Create NUMVERS from VERSION file <c72578>
-
+* 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) <c72578>
 
 
-
 RRDtool 1.8.0 - 2022-03-13
 ==========================
 

diff --git a/configure.ac b/configure.ac
index 11029e4f010dfad5f889140dec67b5ce45853da3..be8b4d6e266b939c75e5268bf0174f256add7fbb 100644 (file)
--- 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 <fcntl.h>])
+AC_CHECK_DECLS([SOCK_CLOEXEC], [], [], [#include <sys/socket.h>])
+
+AC_CACHE_CHECK([whether fopen() supports the "e" flag],[rd_cv_fopen_e],[
+    AC_RUN_IFELSE([
+        AC_LANG_PROGRAM([
+       #include <stdio.h>
+       #include <unistd.h>
+       #include <fcntl.h>
+        ], [
+           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 069f0548603cc30ecad553b6c17c863bc6bd80b7..b116b9719db09bfbef0bc96fb1eb62e683531d49 100644 (file)
--- 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 (file)
index 0000000..6931fa7
--- /dev/null
+++ b/src/compat-cloexec.c
@@ -0,0 +1,101 @@
+#include "compat-cloexec.h"
+
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+
+#ifdef _POSIX_C_SOURCE
+#  include <fcntl.h>
+#  include <unistd.h>
+#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 (file)
index 0000000..0d4d2b6
--- /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 <rrd_config.h>
+
+#ifndef HAVE_DECL_O_CLOEXEC
+#  define O_CLOEXEC 0
+#endif
+
+#ifndef HAVE_DECL_SOCK_CLOEXEC
+#  define SOCK_CLOEXEC 0
+#endif
+
+#include <stdio.h>
+
+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 db293e3ca0f29b7f2360183921ae3eab7e1938a5..b82c310db420b14d9105da1a4ee7072a38fec5b7 100644 (file)
--- a/src/rrd_cgi.c
+++ b/src/rrd_cgi.c
@@ -9,6 +9,8 @@
 #include <stdlib.h>
 #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 faf462ae598e39ed5191f31ccd08bbab55808549..51ba91d66183bbdb33e5f5a769fc06ccb681d471 100644 (file)
--- a/src/rrd_client.c
+++ b/src/rrd_client.c
@@ -66,6 +66,8 @@
 #include <sys/types.h>
 #include <limits.h>
 
+#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 d51a8f3dd866ad6d792057c3db72f5617c907541..0bc296bbc9da079f1acf5e7ea13846e17d6c5b7d 100644 (file)
--- 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 <glib.h>
 /* }}} */
 
@@ -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 c61d645768d8f938f884c4f18fc2817023d4d2fe..264442947325e1125059e8561402c36470e6ed50 100644 (file)
--- 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 b9aa3e2a4f205000fddde516478a194ad15d0809..1e5ca94c72ce5402be95e08c78066fcf67180ace 100644 (file)
--- 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 ed4939a61a02e8f1bf688c97e3f6451a4afcbd80..c326568aa85b68b2c2a87a22c2fb8cd0416ad6bb 100644 (file)
--- 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 b8ca5b6249ff8cc34942679d8882b9982357da22..d15d33dd91ef30a0561cbb973307ef689b564f7d 100644 (file)
--- 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 e434406b20091aee3e7b33a48233cd8a463368a9..8a0dd03070e8aef0c3e231e7f9c3562b86d0b819 100644 (file)
--- 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 5d1b8422a6dd4946cdb268c6fbdc6b2357d02933..2106fa982371992ad92b3131c9612dd120870f05 100644 (file)
--- 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 (file)
index 0000000..297c268
--- /dev/null
+++ b/tests/test_compat-cloexec.c
@@ -0,0 +1,69 @@
+#include <compat-cloexec.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+
+#ifdef _POSIX_C_SOURCE
+#  include <fcntl.h>
+#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 c958ef3e44191151ce03b0a79ce3f2318213dbb9..2a998f30633c672b47f1d2a38dc9de7cd9ef0606 100644 (file)
--- 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\r
 \r
 RRD_LIB_OBJ_LIST = \\r
+        $(TOP)/src/compat-cloexec.obj \\r
         $(TOP)/src/hash_32.obj \\r
         $(TOP)/src/mkstemp.obj \\r
         $(TOP)/src/mutex.obj \\r

diff --git a/win32/Makefile_vcpkg.msc b/win32/Makefile_vcpkg.msc
index 065420b98fde82eb1bbc94da906c7f62cea49919..b3fa78d286b21b86f49be9df3e9f853fd7b564ae 100644 (file)
--- 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\r
 \r
 RRD_LIB_OBJ_LIST = \\r
+        $(TOP)/src/compat-cloexec.obj \\r
         $(TOP)/src/hash_32.obj \\r
         $(TOP)/src/mkstemp.obj \\r
         $(TOP)/src/mutex.obj \\r

diff --git a/win32/librrd-8.def b/win32/librrd-8.def
index f39fb98c81fa17e3b8474505741295cb481eeb98..7f7eaed3e33bbb7f2896417f5abbbba093f8447d 100644 (file)
--- 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 67fdc2dbcdadf358a118b93644fc4e454b05acfb..e3a600b605bcbb4732ea1cddf7926c731c44b3cd 100644 (file)
--- a/win32/librrd-8.vcxproj
+++ b/win32/librrd-8.vcxproj
@@ -384,6 +384,7 @@
     </Link>
   </ItemDefinitionGroup>
   <ItemGroup>
+    <ClCompile Include="..\src\compat-cloexec.c" />
     <ClCompile Include="..\src\hash_32.c" />
     <ClCompile Include="..\src\mkstemp.c" />
     <ClCompile Include="..\src\mutex.c" />