[Raw Msg Headers][Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Race condition in smtpserver?



Hi,

there might be a problem with smtpserver, when it gets too many
connections from the same IP. We had this case yesterday. Smtpserver
closed the connections because there were more than 4*10 connections open.
But when there were definitely no connections to the source, it still
complained to the sender about having 13 connections open. So I suspect a
race condidion betwen the reaper and childregister. I've now changed the
code to completely avoid the fork() and immediately close the connection
when there are more than 4 * MaxSameIpSource connections and added a
sleep(1) in the child. The patch is below, I think it is correct.

Greetings, Swen


diff -ubr zmailer-2.99.49p8.orig/smtpserver/smtpserver.c zmailer-2.99.49p8/smtpserver/smtpserver.c
--- zmailer-2.99.49p8.orig/smtpserver/smtpserver.c	Tue Oct 14 01:21:56 1997
+++ zmailer-2.99.49p8/smtpserver/smtpserver.c	Thu Nov  6 11:06:47 1997
@@ -757,108 +757,111 @@
 
 	    sameipcount = childsameip(&SS.raddr);
 
-	    if ((childpid = fork()) < 0) {	/* can't fork! */
+	    /* We simply -- and FAST -- reject the
+	       remote when it exceeds 4 times the
+	       limit */
+	    if (sameipcount > 4 * MaxSameIpSource) {
 		close(msgfd);
-		fprintf(stderr,
-			"%s: fork(): %s\n",
-			progname, strerror(errno));
-		sleep(5);
-		continue;
-	    } else if (childpid == 0) {		/* child */
-		close(s);	/* Listening socket.. */
-		pid = getpid();
-
-		/* We query, and warn the remote when
-		   the count exceeds the limit, and we
-		   simply -- and FAST -- reject the
-		   remote when it exceeds 4 times the
-		   limit */
-		if (sameipcount > 4 * MaxSameIpSource)
-		    exit(0);
-
-		if (msgfd != 0)
-		    dup2(msgfd, 0);
-		dup2(0, 1);
-		if (msgfd > 1)
+	    } else {
+		if ((childpid = fork()) < 0) {	/* can't fork! */
 		    close(msgfd);
-		msgfd = 0;
-
-		if (logfp)	/* Open the logfp latter.. */
-		    fclose(logfp);
-		logfp = NULL;
-
-		if (maxloadavg != 999 &&
-		    maxloadavg < loadavg_current()) {
-		    write(msgfd, msg_toohighload,
-			  strlen(msg_toohighload));
-		    exit(1);
-		}
-		SIGNAL_HANDLE(SIGTERM, SIG_IGN);
+		    fprintf(stderr,
+			    "%s: fork(): %s\n",
+			    progname, strerror(errno));
+		    sleep(5);
+		    continue;
+		} else if (childpid == 0) {		/* child */
+		    close(s);	/* Listening socket.. */
+		    pid = getpid();
+
+		    if (msgfd != 0)
+			dup2(msgfd, 0);
+		    dup2(0, 1);
+		    if (msgfd > 1)
+			close(msgfd);
+		    msgfd = 0;
+
+		    if (logfp)	/* Open the logfp latter.. */
+			fclose(logfp);
+		    logfp = NULL;
+
+		    if (maxloadavg != 999 &&
+			maxloadavg < loadavg_current()) {
+			write(msgfd, msg_toohighload,
+			      strlen(msg_toohighload));
+			exit(1);
+		    }
+		    SIGNAL_HANDLE(SIGTERM, SIG_IGN);
 
 #if defined(AF_INET6) && defined(INET6)
 
-		if (SS.raddr.v6.sin6_family == AF_INET6)
-		    SS.rport = SS.raddr.v6.sin6_port;
-		else
+		    if (SS.raddr.v6.sin6_family == AF_INET6)
+			SS.rport = SS.raddr.v6.sin6_port;
+		    else
 #endif
-		    SS.rport = SS.raddr.v4.sin_port;
-
-		setrhostname(&SS);
-
-		/* Lets figure-out who we are this time around -- we may be on
-		   a machine with multiple identities per multiple interfaces,
-		   or via virtual IP-numbers, or ... */
-		localsocksize = sizeof(SS.localsock);
-		if (getsockname(msgfd, (struct sockaddr *) &SS.localsock,
-				&localsocksize) != 0) {
-		    /* XX: ERROR! */
-		}
-		zopenlog("smtpserver", LOG_PID, LOG_MAIL);
-
-		s_setup(&SS, msgfd, stdout);
+			SS.rport = SS.raddr.v4.sin_port;
 
-		if (ident_flag != 0)
-		    setrfc1413ident(&SS);
-		else
-		    strcpy(SS.ident_username, "IDENT-NOT-QUERIED");
+		    setrhostname(&SS);
 
-		sprintf(SS.ident_username + strlen(SS.ident_username),
-			" [port %d]", SS.rport);
-
-		if (smtp_syslog && ident_flag)
-		  zsyslog((LOG_INFO, "connection from %s@%s\n",
-			   SS.ident_username, SS.rhostname));
-
-		pid = getpid();
-
-		openlogfp(&SS, daemon_flg);
-		if (logfp != NULL)
-		    fprintf(logfp,
+		    /* Lets figure-out who we are this time around -- we may be on
+		       a machine with multiple identities per multiple interfaces,
+		       or via virtual IP-numbers, or ... */
+		    localsocksize = sizeof(SS.localsock);
+		    if (getsockname(msgfd, (struct sockaddr *) &SS.localsock,
+				    &localsocksize) != 0) {
+			/* XX: ERROR! */
+		    }
+		    zopenlog("smtpserver", LOG_PID, LOG_MAIL);
+
+		    s_setup(&SS, msgfd, stdout);
+
+		    if (ident_flag != 0)
+			setrfc1413ident(&SS);
+		    else
+			strcpy(SS.ident_username, "IDENT-NOT-QUERIED");
+
+		    sprintf(SS.ident_username + strlen(SS.ident_username),
+			    " [port %d]", SS.rport);
+
+		    if (smtp_syslog && ident_flag)
+		      zsyslog((LOG_INFO, "connection from %s@%s\n",
+			       SS.ident_username, SS.rhostname));
+
+		    pid = getpid();
+
+		    openlogfp(&SS, daemon_flg);
+		    if (logfp != NULL)
+			fprintf(logfp,
 		    "%d#\tconnection from %s ident: %s\n",
 		    pid, SS.rhostname, SS.ident_username);
 
 /* if (logfp) fprintf(logfp,"%d#\tInput fd=%d\n",getpid(),msgfd); */
 
-		if (sameipcount > MaxSameIpSource && sameipcount > 1) {
-		    int len;
-		    char msg[200];
-		    sprintf(msg, "450-Too many simultaneous connections from same IP address (%d max %d)\r\n", sameipcount, MaxSameIpSource);
-		    len = strlen(msg);
-		    if (write(msgfd, msg, len) != len)
-			exit(1);	/* Tough.. */
-		    strcpy(msg, "450 Come again latter\r\n");
-		    len = strlen(msg);
-		    write(msgfd, msg, len);
-		    exit(0);	/* Now exit.. */
+		    if (sameipcount > MaxSameIpSource && sameipcount > 1) {
+			int len;
+			char msg[200];
+			sprintf(msg, "450-Too many simultaneous connections from same IP address (%d max %d)\r\n", sameipcount, MaxSameIpSource);
+			len = strlen(msg);
+			if (write(msgfd, msg, len) != len)
+			    exit(1);	/* Tough.. */
+			strcpy(msg, "450 Come again latter\r\n");
+			len = strlen(msg);
+			write(msgfd, msg, len);
+			sleep(1); /* not so fast, but tries to avoid a race
+				   * condition: the child may exit before the
+				   * parent calls childregister - bummer */
+			exit(0);	/* Now exit.. */
+		    }
+		    smtpserver(&SS, 1);
+
+		    if (routerpid > 0)
+			killr(&SS, routerpid);
+		    _exit(0);
+		} else {
+		    close(msgfd);
 		}
-		smtpserver(&SS, 1);
-
-		if (routerpid > 0)
-		    killr(&SS, routerpid);
-		_exit(0);
-	    } else
-		close(msgfd);
-	    childregister(childpid, &SS.raddr);
+		childregister(childpid, &SS.raddr);
+	    }
 	}
     }
 #else				/* !USE_INET */