Yahoo Groups archive

Milter-greylist

Index last updated: 2026-04-28 23:32 UTC

Thread

[PATCH] Set CLOEXEC flags for sockets

[PATCH] Set CLOEXEC flags for sockets

2010-06-09 by Enrico Scholz

Set CLOEXEC flags for sockets

Subprocesses spawned by 'stat "| ..."' inherited all open sockets.
This wastes resources because it keeps lot of half-open sockets in the
system, can cause problems with SELinux and cause misbehavior because
sockets seems to be still open for the other side.

E.g. on my system, the stat logger consumes

 # ls /proc/10204/fd | wc -l
 166

sockets.

Index: milter-greylist/milter-greylist.h
===================================================================
--- milter-greylist.orig/milter-greylist.h
+++ milter-greylist/milter-greylist.h
@@ -270,6 +270,16 @@ char *fstring_escape(char *);
 size_t mystrlcat(char *, const char *src, size_t size);
 #endif
 
+#ifdef USE_CLOEXEC
+/* This requires Linux 2.6.27+ and the conditional must be set manually */
+#define socket_cloexec(_domain, _type, _protocol) \
+	socket(_domain, (_type) | SOCK_CLOEXEC, _protocol)
+#else
+int socket_cloexec(int domain, int type, int protocol);
+#endif
+
+int set_cloexec_flag(int fd, int value);
+
 /*
  * Locking management
  */
Index: milter-greylist/p0f.c
===================================================================
--- milter-greylist.orig/p0f.c
+++ milter-greylist/p0f.c
@@ -268,7 +268,7 @@ p0f_connect(void)
 	if (!conf.c_p0fsock[0])
 		return -1;
 
-	if ((p0fsock = socket(PF_UNIX,SOCK_STREAM,0)) == -1) {
+	if ((p0fsock = socket_cloexec(PF_UNIX,SOCK_STREAM,0)) == -1) {
 		mg_log(LOG_ERR, "socket(PF_UNIX, SOCK_STREAM, 0) failed");
 		exit(EX_OSERR);
 	}
Index: milter-greylist/spamd.c
===================================================================
--- milter-greylist.orig/spamd.c
+++ milter-greylist/spamd.c
@@ -441,7 +441,7 @@ spamd_unix_socket(path)
 	sun.sun_family = AF_UNIX;
 	strncpy(sun.sun_path, path, sizeof(sun.sun_path) - 1);
 
-	if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
+	if ((sock = socket_cloexec(AF_UNIX, SOCK_STREAM, 0)) == -1) {
 		mg_log(LOG_ERR, "spamd socket failed: %s", strerror(errno));
 		return -1;
 	}
@@ -478,9 +478,9 @@ spamd_inet_socket(host, port)
 	}
 
 	for (res = ai; res != NULL; res = res->ai_next) {
-		sock = socket(res->ai_family, 
-			      res->ai_socktype, 
-			      res->ai_protocol);
+		sock = socket_cloexec(res->ai_family,
+				      res->ai_socktype,
+				      res->ai_protocol);
 		if (sock == -1)
 			continue;
 
Index: milter-greylist/sync.c
===================================================================
--- milter-greylist.orig/sync.c
+++ milter-greylist/sync.c
@@ -456,7 +456,8 @@ peer_connect(peer)	/* peer list is read-
 
 	for (res = res0; res; res = res->ai_next) {
 		/*We only test an address family which kernel supports. */
-		s = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
+		s = socket_cloexec(res->ai_family, res->ai_socktype,
+				   res->ai_protocol);
 		if (s == -1)
 			continue;
 		close(s);
@@ -469,7 +470,8 @@ peer_connect(peer)	/* peer list is read-
 	}
 
 	for (res = res0; res; res = res->ai_next) {
-		s = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
+		s = socket_cloexec(res->ai_family, res->ai_socktype,
+				   res->ai_protocol);
 		if (s == -1)
 			continue;
 
@@ -548,7 +550,8 @@ peer_connect(peer)	/* peer list is read-
 	else
 		proto = pe->p_proto;
 
-	if ((s = socket(SA(&raddr)->sa_family, SOCK_STREAM, proto)) == -1) {
+	if ((s = socket_cloexec(SA(&raddr)->sa_family, SOCK_STREAM,
+				proto)) == -1) {
 		mg_log(LOG_ERR, "cannot sync with peer %s, "
 		    "socket failed: %s (%d entries queued)", 
 		    peer->p_name, strerror(errno), peer->p_qlen);
@@ -786,6 +789,7 @@ sync_master(arg)
 
 
 		}
+		set_cloexec_flag(fd, 1);
 		unmappedaddr(SA(&raddr), &raddrlen);
 
 		conf_release();
@@ -952,7 +956,7 @@ sync_listen(addr, port, sms)
 		return;
 	}
 
-	if ((s = socket(SA(&laddr)->sa_family, SOCK_STREAM, proto)) == -1) {
+	if ((s = socket_cloexec(SA(&laddr)->sa_family, SOCK_STREAM, proto)) == -1) {
 		sms->runs = SMS_DISABLED;
 		return;
 	}
@@ -1515,7 +1519,7 @@ local_addr(sa, salen)
 		break;
 	}
 
-	if ((sfd = socket(sa->sa_family, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
+	if ((sfd = socket_cloexec(sa->sa_family, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
 		mg_log(LOG_ERR, "local_addr: socket failed: %s",
 		    strerror(errno));
 		return -1;
Index: milter-greylist/conf.c
===================================================================
--- milter-greylist.orig/conf.c
+++ milter-greylist/conf.c
@@ -185,6 +185,7 @@ conf_load_internal(timestamp)
 		if (conf_cold)
 			exit(EX_OSERR);
 	} else {
+		set_cloexec_flag(fileno(stream), 1);
 		TSS_SET(conf_key, newconf);
 
 		peer_clear();
Index: milter-greylist/fd_pool.c
===================================================================
--- milter-greylist.orig/fd_pool.c
+++ milter-greylist/fd_pool.c
@@ -122,6 +122,7 @@ int fd_new_desc() {
                         strerror(errno));
                 return -1;
         }
+	set_cloexec_flag(descriptor, 1);
 	return descriptor;
 }
 
@@ -340,6 +341,7 @@ FILE *fopen_ext(char *path, char *mode) 
 	err = errno;
 
 	if (stream != NULL) {
+		set_cloexec_flag(fileno(stream), 1);
 		if ( descriptor == fileno(stream) ) {
 			/* we are in luck, fopen has successfully aquired our low descriptor ... */
 			return stream;
Index: milter-greylist/milter-greylist.c
===================================================================
--- milter-greylist.orig/milter-greylist.c
+++ milter-greylist/milter-greylist.c
@@ -3386,3 +3386,29 @@ mg_setreply(ctx, priv, rcpt)
 	return r;
 }
 
+#ifndef USE_CLOEXEC
+int socket_cloexec(int domain, int type, int protocol)
+{
+	int		fd = socket(domain, type, protocol);
+
+	if (fd >= 0)
+		set_cloexec_flag(fd, 1);
+
+	return fd;
+}
+#endif
+
+int set_cloexec_flag (int fd, int value)
+{
+	int oldflags = fcntl(fd, F_GETFD, 0);
+
+	if (oldflags < 0)
+		return oldflags;
+
+	if (value)
+		oldflags |= FD_CLOEXEC;
+	else
+		oldflags &= ~FD_CLOEXEC;
+
+	return fcntl(fd, F_SETFD, oldflags);
+}
Index: milter-greylist/stat.c
===================================================================
--- milter-greylist.orig/stat.c
+++ milter-greylist/stat.c
@@ -126,6 +126,8 @@ mg_stat_def(output, fstring)
 		return;
 	}
 
+	set_cloexec_flag(fileno(outfp), 1);
+
 	if ((format = fstring_escape(strdup(fstring))) == NULL) {
 		mg_log(LOG_ERR, "strdup failed: %s", strerror(errno));
 		exit(EX_OSERR);

Re: [milter-greylist] [PATCH] Set CLOEXEC flags for sockets

2010-06-10 by manu@netbsd.org

Enrico Scholz <enrico.scholz@...-chemnitz.de> wrote:

> +#ifdef USE_CLOEXEC

Is there a reason why you don't you just include <fcntl.h> and test
#ifdef FD_CLOEXEC ?


> -     if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
> +     if ((sock = socket_cloexec(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>           mg_log(LOG_ERR, "spamd socket failed: %s", strerror(errno));
>           return -1;

I think I would rather do something like this:

        if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
            mg_log(LOG_ERR, "spamd socket failed: %s", strerror(errno));
            return -1;
        }
        SET_CLOEXEC(sock);
        
With

#ifdef FD_CLOEXEC
#define SET_CLOEXEC(fd)  do {                           \
        int flags = fcntl((fd), F_GETFD, 0);                    \
                                                        \
        if (flags != -1)                                        \
                (void)fcntl((fd, F_SETFD, flags|FD_CLOEXEC);    \
} while (0 /* CONSTCOND */);
#else /* FD_CLOEXEC */
#define SET_CLOEXEC(fd)
#endif /* FD_CLOEXEC */

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Re: [PATCH] Set CLOEXEC flags for sockets

2010-06-10 by Enrico Scholz

manu-S783fYmB3Ccdnm+yROfE0A@... writes:

>> +#ifdef USE_CLOEXEC
>
> Is there a reason why you don't you just include <fcntl.h> and test
> #ifdef FD_CLOEXEC ?

The wrapped

  socket(_domain, (_type) | SOCK_CLOEXEC, _protocol)
                         ~~~~~~~~~~~~~~~

call is very runtime specific (requires Linux 2.6.27+) and there are
major Linux distributions which ship older kernels.  Doing a buildtime
check seems too risky for me.


>> -     if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>> +     if ((sock = socket_cloexec(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>>           mg_log(LOG_ERR, "spamd socket failed: %s", strerror(errno));
>>           return -1;
>
> I think I would rather do something like this:
>
>         if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>             mg_log(LOG_ERR, "spamd socket failed: %s", strerror(errno));
>             return -1;
>         }
>         SET_CLOEXEC(sock);

your decision... I used the socket_cloexec() to generate more efficient
code (especially when SOCK_CLOEXEC works).


Enrico

Re: [milter-greylist] Re: [PATCH] Set CLOEXEC flags for sockets

2010-06-10 by Emmanuel Dreyfus

On Thu, Jun 10, 2010 at 09:54:53AM +0200, Enrico Scholz wrote:
>   socket(_domain, (_type) | SOCK_CLOEXEC, _protocol)
> 
> call is very runtime specific (requires Linux 2.6.27+) and there are
> major Linux distributions which ship older kernels.  Doing a buildtime
> check seems too risky for me.

You worry about binary packages? If they are properly done, they should
take care of that. I don't think anyone should tweak its code to cope with
poor package policies.

We can ifdef SOCK_CLOEXEC, though.

> your decision... I used the socket_cloexec() to generate more efficient
> code (especially when SOCK_CLOEXEC works).

I'm not sure that adding a function call more efficient than using 
a macro.

-- 
Emmanuel Dreyfus
manu@...

Re: [milter-greylist] [PATCH] Set CLOEXEC flags for sockets

2010-06-16 by manu@netbsd.org

Enrico Scholz <enrico.scholz@...-chemnitz.de> wrote:

> Set CLOEXEC flags for sockets

I checked in the patch (with the modifications I posted to the list)
-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Move to quarantaine

This moves the raw source file on disk only. The archive index is not changed automatically, so you still need to run a manual refresh afterward.