3rd set of fixes for apache-fastcgi-2.0b1

Sonya Rikhtverchik (rikhtver@OpenMarket.com)
Tue, 02 Sep 1997 11:23:53 -0400

Message-Id: <199709021523.LAA00505@u4-138.openmarket.com>
To: fastcgi-developers@OpenMarket.com
Subject: 3rd set of fixes for apache-fastcgi-2.0b1
Date: Tue, 02 Sep 1997 11:23:53 -0400
From: Sonya Rikhtverchik <rikhtver@OpenMarket.com>



Date: Tue, 26 Aug 1997 12:37:51 -0400 (EDT)
Message-Id: <MAA02350.199708261637@bastille.va.pubnix.com>
From: "David J. MacKenzie" <djm@va.pubnix.com>
To: fastcgi-developers@openmarket.com
cc: djm@va.pubnix.com
Subject: 3rd set of fixes for apache-fastcgi-2.0b1

This patch fixes the problems noted below.  If anyone is reading this
list and maintaining the apache fastcgi module, could you send me an
acknowledgement, please?

I am now stuck at the problem that dynamic CGI programs can't restart
after being killed.  The socket and lock file don't get removed, so
the connect() fails with "Connection refused" (naturally, if the
process on the other end of the socket was killed).  Help!

Problems fixed by this patch:

1. Erroneous fprintf arguments and a missing return value, all
detected by gcc -Wall.

2. Duplicated code to create the lock file name merged into a single
function, closing several memory leaks.

3. FCGIConfig -minProcesses didn't allow a value of 0, so the last
FCGI app in a given dynamic class would live forever, even if it
hadn't been requested for weeks.

4. The test for whether to keep looking for dynamic app victims to
kill was backwards, so no dynamic apps were ever selected as victims.

5. The killInterval and updateInterval were being ignored; the
sigsuspend() forced recalculations only when a child died, instead of
at the intervals specified.

6. When a dynamic app couldn't be started, the program SEGV'd when
trying to free the ipcAddrPtr twice.

- --- mod_fastcgi.c	1997/08/25 22:01:22	1.3
+++ mod_fastcgi.c	1997/08/26 16:26:06
@@ -930,6 +930,7 @@
         ipcAddrPtr->addrType = TYPE_UNKNOWN;
         ipcAddrPtr->addrLen = 0;
     }
+    return -1;
 }
 
 /*
@@ -1190,9 +1191,9 @@
      */
     errorLogFile = getErrLog();
     fprintf(errorLogFile,
- -            "[%s] mod_fastcgi: %s pid %d syscall %s failed"
+            "[%s] mod_fastcgi: %s pid %ld syscall %s failed"
             " before entering app, errno = %s.\n",
- -            get_time(), programName, getpid(), failedSysCall,
+            get_time(), programName, (long) getpid(), failedSysCall,
             strerror(save_errno));
     fflush(errorLogFile);
     exit(save_errno);
@@ -2351,6 +2352,26 @@
 }
 
 /*
+ * MakeLockFileName
+ *
+ * Returns the lock file name for application execName.
+ */
+
+char *
+MakeLockFileName(char *execName)
+{
+  char *hashedPath, *lockName;
+
+  hashedPath = GetHashedPath(execName);
+  lockName = Malloc(strlen(ipcDynamicDir)+strlen(hashedPath)+7);
+  strcpy(lockName, ipcDynamicDir);
+  strcat(lockName, "/");
+  strcat(lockName, hashedPath);
+  strcat(lockName, ".lock");
+  return lockName;
+}
+
+/*
  *----------------------------------------------------------------------
  *
  * LookupFcgiServerInfo --
@@ -3268,7 +3289,7 @@
             }
             i++;
 	    n = strtol(argv[i], &cvtPtr, 10);
- -	    if(*cvtPtr != '\0' || n < 1 || n > 250) {
+	    if(*cvtPtr != '\0' || n < 0 || n > 250) {
                 goto BadValueReturn;
             }
 	    minProcs = n;
@@ -3485,11 +3506,11 @@
                 id, execPath);
                 break;
         case CONN_TIMEOUT: 
- -                sprintf(buf, "%c %s %ul\n",
+                sprintf(buf, "%c %s %lu\n",
                 id, execPath, qsecs);
                 break;
         case REQ_COMPLETE:  
- -                sprintf(buf, "%c %s %ul %ul %ul\n",
+                sprintf(buf, "%c %s %lu %lu %lu\n",
                 id, execPath, qsecs, ctime, now);
                 break;
     }
@@ -3619,10 +3640,10 @@
 	            sscanf(ptr1, "%c %s\n", &opcode, execName);
 	            break;
 	        case CONN_TIMEOUT:
- -	            sscanf(ptr1, "%c %s %ul\n", &opcode, execName, &qsec);
+	            sscanf(ptr1, "%c %s %lu\n", &opcode, execName, &qsec);
 		    break;
 		case REQ_COMPLETE:
- -	            sscanf(ptr1, "%c %s %ul %ul %ul\n", &opcode, execName,
+	            sscanf(ptr1, "%c %s %lu %lu %lu\n", &opcode, execName,
 		            &qsec, &ctime, &now);
 		    break;
 	        default:
@@ -3659,15 +3680,10 @@
 		}
 		OS_FreeIpcAddr(ipcAddrPtr);
 		/* don't forget to create a lock file for this app */
- -		buf2 = Malloc(strlen(GetHashedPath(execName))+
- -                        strlen(ipcDynamicDir)+7);
- -		ASSERT(buf2!=NULL);
- -		strcpy(buf2, ipcDynamicDir);
- -		strcat(buf2, "/");
- -		strcat(buf2, GetHashedPath(execName));
- -		strcat(buf2, ".lock");
+		buf2 = MakeLockFileName(execName);
 		fd = open(buf2, O_WRONLY | O_CREAT | O_TRUNC, 
 		        S_IRUSR | S_IWUSR);
+		free(buf2);
 		ASSERT(fd>0);
 		close(fd);
 	    } else {
@@ -3806,7 +3822,10 @@
 
     /* pass 1 - locate and mark all victims */
     for(s=fastCgiServers;  s!=NULL; s=s->next) {
- -        if((globalNumInstances-victims)>minProcs) {
+        /* If the number of non-victims is less than or equal to
+	   the minimum that may be running without being killed off,
+	   don't select any more victims.  */
+        if((globalNumInstances-victims)<=minProcs) {
 	    break;
 	}
         totalTime = (s->numProcesses)*(now-epoch)*1000000;
@@ -3836,14 +3855,7 @@
     for(s=fastCgiServers; s!=NULL; s=s->next) {
         for(i=0;i<maxClassProcs;i++) {
 	    if(s->procInfo[i].state == STATE_VICTIM) {
- -	        len = strlen(ipcDynamicDir); 
- -	        len += (strlen(GetHashedPath(DStringValue(&s->execPath)))+1);
- -		lockFileName = Malloc(len+6);
- -		strcpy(lockFileName, ipcDynamicDir);
- -		strcat(lockFileName, "/");
- -		strcat(lockFileName, 
- -                        GetHashedPath(DStringValue(&s->execPath)));
- -		strcat(lockFileName, ".lock");
+	        lockFileName = MakeLockFileName(DStringValue(&s->execPath));
 		if((lockFd = open(lockFileName, O_RDWR))<0) {
 		    /* 
 		     * If we need to kill an application and the
@@ -3863,9 +3875,7 @@
 		     * situation occurs very rarely, which it should
 		     */
 		    funcData = Malloc(sizeof(struct FuncData));
- -		    funcData->lockFileName = Malloc(
- -		            strlen(lockFileName)+1);
- -		    strcpy(funcData->lockFileName, lockFileName);
+		    funcData->lockFileName = lockFileName;
 		    /* 
 		     * We can not call onto spawn_child() here
 		     * since we are completely disassociated from
@@ -3955,7 +3965,7 @@
         caughtSigTerm = TRUE;
     } else if(signo == SIGCHLD) {
         caughtSigChld = TRUE;
- -    } else if(signo == SIGUSR2) {
+    } else if(signo == SIGUSR2 || signo == SIGALRM) {
         caughtSigUsr1 = TRUE;
     }
 }
@@ -3991,8 +4001,7 @@
      */
     if(geteuid() == 0 && setuid(user_id) == -1) {
         fprintf(errorLogFile,
- -                "[%s] mod_fastcgi: Unable to change uid\n",
- -		" exiting\n",
+                "[%s] mod_fastcgi: Unable to change uid, exiting\n",
               get_time());
         fflush(errorLogFile);
         exit(1);
@@ -4037,7 +4046,7 @@
      */
     for (;;) {
         time_t now;
- -        int sleepSeconds = INT_MAX;
+        int sleepSeconds = min(killInterval, updateInterval);
         pid_t childPid;
         int waitStatus;
 	int numChildren;
@@ -4212,7 +4221,7 @@
 	        }
 	    }
 	    /*
- -	     * If we get to this point, than, it indicates the
+	     * If we get to this point, we have detected the
 	     * termination of the process that was spawned off by
 	     * the process manager to do a blocking kill above.
 	     */
@@ -5517,7 +5526,6 @@
     FastCgiInfo *infoPtr = NULL;
     OS_IpcAddr *ipcAddrPtr = NULL;
     struct sockaddr_un* addrPtr = NULL;
- -    DString fileName;
     char *msg = NULL;
     char *argv0 = NULL;
     uid_t uid;
@@ -5525,9 +5533,9 @@
     struct timeval start, ctime, qtime;
     struct timeval tval;
     fd_set write_fds;
- -    int status, flags;
+    int status, flags = 0;
     int dynamic = FALSE, result;
- -    int lockFd;
+    int lockFd = 0;
 
     /* 
      * Model after mod_cgi::cgi_handler() to provide better 
@@ -5645,13 +5653,9 @@
      * Open a connection to the FastCGI application.
      */
     if(dynamic==TRUE) {
- -        DStringInit(&fileName);
- -	DStringAppend(&fileName, ipcDynamicDir, -1);
- -	DStringAppend(&fileName, "/", -1);
- -	DStringAppend(&fileName, GetHashedPath(reqPtr->filename), -1);
- -	DStringAppend(&fileName, ".lock", -1);
+        char *lockFileName = MakeLockFileName(reqPtr->filename);
 	do {
- -	    result = DoesLockExist(DStringValue(&fileName));
+	    result = DoesLockExist(lockFileName);
 	    switch (result) {
 	        case -1:
 	            sprintf(infoPtr->errorMsg,
@@ -5663,7 +5667,7 @@
 		    sleep(1);
                     break;
 	        case 1:
- -   	    	    lockFd = open(DStringValue(&fileName),O_APPEND);
+   	    	    lockFd = open(lockFileName,O_APPEND);
 		    result = (lockFd<0)?(0):(1);
 		    break;
 	    }
@@ -5673,13 +5677,14 @@
                     "mod_fastcgi: Can't obtain a read lock");
 	    goto ErrorReturn;
 	}
+	free(lockFileName);
     }
 
     /* create connection point */
     if(dynamic==TRUE) {
         ipcAddrPtr = (OS_IpcAddr *) OS_InitIpcAddr();
 	/* need to fill in the serverAddr structure, even 
- -	   though, the process manager is the one actually
+	   though the process manager is the one actually
 	   responsible for creating a socket */
 	addrPtr = (struct sockaddr_un *) Malloc(sizeof(struct sockaddr_un));
 	ipcAddrPtr->serverAddr = (struct sockaddr *) addrPtr;
@@ -5690,6 +5695,7 @@
         }  
 	ipcAddrPtr->addrType = TYPE_LOCAL;
     } else {
+        ASSERT(serverInfoPtr != NULL);
         ipcAddrPtr = (OS_IpcAddr *) serverInfoPtr->ipcAddr;
     }
     if((infoPtr->fd = OS_Socket(ipcAddrPtr->serverAddr->sa_family, 
@@ -5813,7 +5819,9 @@
         msg = "errno out of range";
     }
     Free(infoPtr->errorMsg);
- -    OS_FreeIpcAddr(ipcAddrPtr);
+    if (dynamic!=TRUE) {
+        OS_FreeIpcAddr(ipcAddrPtr);
+    }
     infoPtr->errorMsg = Malloc(FCGI_ERRMSG_LEN + strlen(msg));
     sprintf(infoPtr->errorMsg,
             "mod_fastcgi: Could not connect to application,"

------- End of Forwarded Message