4th set of fixes for apache-fastcgi-2.0b1

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

Message-Id: <199709021525.LAA00515@u4-138.openmarket.com>
To: fastcgi-developers@OpenMarket.com
Subject: 4th set of fixes for apache-fastcgi-2.0b1
Date: Tue, 02 Sep 1997 11:25:14 -0400
From: Sonya Rikhtverchik <rikhtver@OpenMarket.com>



Date: Wed, 27 Aug 1997 18:25:14 -0400 (EDT)
Message-Id: <SAA22271.199708272225@bastille.va.pubnix.com>
From: "David J. MacKenzie" <djm@va.pubnix.com>
To: fastcgi-developers@openmarket.com
cc: djm@va.pubnix.com
Subject: 4th set of fixes for apache-fastcgi-2.0b1

This patch fixes the problems noted below.  

Problems fixed by this patch, which should be applied after the others
I've sent:

1. Remove the dead lock file and socket when cleaning up after a
server whose last child has been killed, so the FastCGIHandler isn't
fooled into thinking there is still a process serving that app.

2. Plug a memory leak when doing a blocking kill.

3. Make file locking robust in the presence of signals.

4. Rename some badly named variables.

5. Fixes typos in many comments.

- --- mod_fastcgi.c	1997/08/26 17:16:51	1.4
+++ mod_fastcgi.c	1997/08/27 22:04:36
@@ -818,7 +818,7 @@
         goto GET_IPC_ERROR_EXIT;
     }
 #ifndef __EMX__
- -     /* OS/2 dosen't support changing ownership. */
+     /* OS/2 doesn't support changing ownership. */
     chown(DStringValue(&ipcAddrPtr->bindPath), uid, gid);
 #endif    
 
@@ -2354,7 +2354,8 @@
 /*
  * MakeLockFileName
  *
- - * Returns the lock file name for application execName.
+ * Returns the lock file name for dynamic application execName.
+ * Allocates memory for it.
  */
 
 char *
@@ -2504,6 +2505,9 @@
 {
     FcgiProcessInfo *processInfoPtr;
     int i;
+    char *lockFileName;
+    OS_IpcAddr *ipcAddrPtr;
+    char *fname;
 
     /*
      * Free up process/connection info.
@@ -2516,6 +2520,22 @@
         }
         OS_FreeIpcAddr(processInfoPtr->ipcAddr);
     }
+
+    /*
+     * Remove the dead lock file and socket.
+     */
+    lockFileName = MakeLockFileName(DStringValue(&serverInfoPtr->execPath));
+    unlink(lockFileName);
+    Free(lockFileName);
+
+    ipcAddrPtr = (OS_IpcAddr *)serverInfoPtr->ipcAddr;
+    fname = MakeSocketName(DStringValue(&serverInfoPtr->execPath),
+			   NULL, -1, &ipcAddrPtr->bindPath, 1);
+    /* Remove extraneous digit from end.  XXX why is this necessary?  */
+    fname[strlen(fname)-1] = '\0';
+    unlink(fname);
+    Free(fname);
+
     /*
      * Clean up server info structure resources.
      */
@@ -3451,25 +3471,41 @@
 int LockRegion(int fd, int cmd, int type, off_t offset, int whence, off_t len)
 {
     struct flock lock;
+    int res;
     
     lock.l_type = type;       /* F_RDLCK, F_WRLCK, F_UNLCK */
     lock.l_start = offset;    /* byte offset, relative to whence */
     lock.l_whence = whence;   /* SEEK_SET, SEET_CUR, SEEK_END */
     lock.l_len = len;         /* # of bytes, (0 indicates to EOF) */
 
- -    return (fcntl(fd, cmd, &lock)); 
+    /* Don't be fooled into thinking we've set a lock when we've
+       merely caught a signal.  */
+    while ((res = fcntl(fd, cmd, &lock)) == -1 && errno == EINTR)
+      ;
+    return res;
 }
+/* Ways to lock an entire file... */
+
+/* Shared locks: allows other shared locks, but no exclusive locks.  */
+/* Set a shared lock, no wait, failure->errno==EACCES.  */
 #define ReadLock(fd) LockRegion(fd, F_SETLK, F_RDLCK, 0, SEEK_SET, 0)
+/* Set a shared lock, wait until you have it.  */
 #define ReadwLock(fd) LockRegion(fd, F_SETLKW, F_RDLCK, 0, SEEK_SET, 0)
+
+/* Exclusive locks: allows no other locks.  */
+/* Set an exclusive lock, no wait, failure->errno==EACCES.  */
 #define WriteLock(fd) LockRegion(fd, F_SETLK, F_WRLCK, 0, SEEK_SET, 0)
+/* Set a shared lock, wait until you have it.  */
 #define WritewLock(fd) LockRegion(fd, F_SETLKW, F_WRLCK, 0, SEEK_SET, 0)
+
+/* Remove a shared or exclusive lock, no wait, failure->errno=EACCES.  */
 #define Unlock(fd) LockRegion(fd, F_SETLK, F_UNLCK, 0, SEEK_SET, 0)
 
 /****************************************************************************/
 /*
  * Opcodes for Server/ProcMgr communication
  */
- -#define PLEASE_START 49        /* start application */
+#define PLEASE_START 49        /* start dynamic application */
 #define CONN_TIMEOUT 50        /* start another copy of application */
 #define REQ_COMPLETE 51        /* do some data analysis */
 /*
@@ -3560,7 +3596,7 @@
     struct stat statbuf;
     int recs = 0, fd, i;
     char *buf=NULL, opcode;
- -    char *buf2=NULL;
+    char *lockFileName=NULL;
     char *ptr1=NULL, *ptr2=NULL;
     char execName[TMP_BUFSIZ];
     unsigned long qsec = 0, ctime = 0, now = time(NULL);
@@ -3680,12 +3716,12 @@
 		}
 		OS_FreeIpcAddr(ipcAddrPtr);
 		/* don't forget to create a lock file for this app */
- -		buf2 = MakeLockFileName(execName);
- -		fd = open(buf2, O_WRONLY | O_CREAT | O_TRUNC, 
+		lockFileName = MakeLockFileName(execName);
+		fd = open(lockFileName, O_WRONLY | O_CREAT | O_TRUNC, 
 		        S_IRUSR | S_IWUSR);
- -		free(buf2);
 		ASSERT(fd>0);
 		close(fd);
+		free(lockFileName);
 	    } else {
 	        if(opcode==PLEASE_START) {
 		    /* repeated calls to PLEASE_START, ignore */
@@ -3859,7 +3895,7 @@
 		if((lockFd = open(lockFileName, O_RDWR))<0) {
 		    /* 
 		     * If we need to kill an application and the
- -		     * corresponding lock file does not exists, then
+		     * corresponding lock file does not exist, then
 		     * that means we are in big trouble here 
 		     */
 		    continue;
@@ -3883,18 +3919,21 @@
 		     * directly.
 		     */
 		    if((pid=fork())<0) {
- -		        Unlock(lockFd);
+			Free(funcData->lockFileName);
+			Free(funcData);
 			return;
 		    } else if(pid==0) {
 		        /* child */
 		        BlockingKill(funcData);
 		    } else {
 		        /* parent */
+			Free(funcData->lockFileName);
+			Free(funcData);
 		    }
 		} else {
 		    kill(s->procInfo[i].pid, SIGTERM);
 		    Unlock(lockFd);
- -	        break;
+		    break;
 	        }
 	    } 
 	}
@@ -3935,7 +3974,7 @@
  */
 static int caughtSigTerm = FALSE;
 static int caughtSigChld = FALSE;
- -static int caughtSigUsr1 = FALSE;
+static int caughtSigUsr2 = FALSE;
 static char *errorLogPathname = NULL;
 static sigset_t signalsToBlock;
 
@@ -3966,7 +4005,7 @@
     } else if(signo == SIGCHLD) {
         caughtSigChld = TRUE;
     } else if(signo == SIGUSR2 || signo == SIGALRM) {
- -        caughtSigUsr1 = TRUE;
+        caughtSigUsr2 = TRUE;
     }
 }
 
@@ -4146,7 +4185,7 @@
         if(caughtSigTerm) {
             goto ProcessSigTerm;
 	}
- -        if((!caughtSigChld) && (!caughtSigUsr1)) {
+        if((!caughtSigChld) && (!caughtSigUsr2)) {
             /*
              * Enable signals and wait.  The call to sigsuspend
              * breaks the critical region into two, so caughtSigChld
@@ -4158,8 +4197,8 @@
 	}
         callWaitPid = caughtSigChld;
         caughtSigChld = FALSE;
- -	callDynamicProcs = caughtSigUsr1;
- -	caughtSigUsr1 = FALSE;
+	callDynamicProcs = caughtSigUsr2;
+	caughtSigUsr2 = FALSE;
         sigprocmask(SIG_UNBLOCK, &signalsToBlock, NULL);
 
         /*
@@ -4183,9 +4222,6 @@
 	}
 
         if(!callWaitPid) {
- -            /*
- -             * Must be time to restart somebody.
- -             */
             continue;
 	}
 
@@ -4342,8 +4378,8 @@
  * DoesAnyFileExist --
  *
  *      Check if the file is present in the file system in the
- - *      directory specified by $ircDynamicDir.  Also check that
- - *      a given filename of the certain type, such as regular or
+ *      directory specified by ipcDynamicDir.  Also check that
+ *      a given filename is of the given type, such as regular or
  *      socket (FIFO).
  *
  * Results:
@@ -4413,11 +4449,11 @@
  *      An Apache module initializer, called by the Apache core
  *      after reading the server config.
  *
- - *      Start the process manager no matter what, since there maybe a 
+ *      Start the process manager no matter what, since there may be a 
  *      request for dynamic FastCGI applications without any being 
  *      configured as static applications.  Also, check for the existence
  *      and create if necessary a subdirectory into which all dynamic
- - *      sockets will go into.
+ *      sockets will go.
  *
  *----------------------------------------------------------------------
  */
@@ -5589,7 +5625,7 @@
     if (serverInfoPtr == NULL) {
         /* 
          * Either this application does not exist, or it is 
- -	 * the invokation of the dynamic fastcgi 
+	 * the invocation of the dynamic fastcgi 
 	 * application.
 	 */
         uid = (user_id == (uid_t) -1)  ? geteuid() : user_id;

------- End of Forwarded Message