Fix the unixfork/unixcomm code to deal with pid over 65535 (#333)

* ForkUnixShell() can be a static procedure

* Restructure SAFEREAD() to be a little clearer

* Convert unixjob type field defines to an enum and fix related unhandled switch cases.

* Use local declaration of loop variables in for-loop, removing register attributes

* Restructure SAFEREAD() to be a little clearer

* Send and receive 2 additional bytes for pid.

* Prefer standard C99 "inline" over "__inline__" for SAFEREAD

inc/unixfork.h

@@ -1,5 +1,4 @@
 #ifndef UNIXFORK_H
 #define UNIXFORK_H 1
 int fork_Unix(void);
-int ForkUnixShell(int slot, char *PtySlave, char *termtype, char *shellarg);
 #endif /* UNIXFORK_H */

src/unixcomm.c

@@ -56,14 +56,13 @@ Unix Interface Communications
 #include "byteswapdefs.h"
 #include "commondefs.h"
 
-static __inline__ int SAFEREAD(int f, unsigned char *b, int c) {
+static inline ssize_t SAFEREAD(int f, unsigned char *b, int c) {
-  int res;
+  ssize_t res;
-loop:
+  do {
-  res = read(f, b, c);
+    res = read(f, b, c);
-  if ((res < 0)) {
+    if (res >= 0) return (res);
-    if (errno == EINTR || errno == EAGAIN) goto loop;
+  } while (errno == EINTR || errno == EAGAIN);
-    perror("reading UnixPipeIn");
+  perror("reading UnixPipeIn");
-  }
   return (res);
 }
 
@@ -79,11 +78,13 @@ int NPROCS = 100;
 /* One of these structures exists for every possible file descriptor */
 /* type field encodes kind of stream:                                */
 
-#define UJUNUSED 0    /* Unused */
+enum UJTYPE {
-#define UJSHELL -1    /* PTY shell */
+  UJUNUSED = 0,
-#define UJPROCESS -2  /* random process */
+  UJSHELL = -1,   /* PTY shell */
-#define UJSOCKET -3   /* socket open for connections */
+  UJPROCESS = -2, /* random process */
-#define UJSOSTREAM -4 /* connection from a UJSOCKET */
+  UJSOCKET = -3,  /* socket open for connections */
+  UJSOSTREAM = -4 /* connection from a UJSOCKET */
+};
 
 /* These are indexed by WRITE socket# */
 struct unixjob {
@@ -91,14 +92,14 @@ struct unixjob {
   int readsock;   /* Socket to READ from for this job. */
   int PID;        /* process ID associated with this slot */
   int status;     /* status returned by subprocess (not shell) */
-  int type;
+  enum UJTYPE type;
 };
 
 struct unixjob *UJ; /* allocated at run time */
 
 long StartTime; /* Time, for creating pipe filenames */
 
-#define valid_slot(slot) ((slot) >= 0 && (slot) < NPROCS && UJ[slot].type)
+#define valid_slot(slot) ((slot) >= 0 && (slot) < NPROCS && UJ[slot].type != UJUNUSED)
 
 char shcom[2048]; /* Here because I'm suspicious of */
                   /* large allocations on the stack */
@@ -117,9 +118,7 @@ int find_process_slot(register int pid)
 /* Find a slot with the specified pid */
 
 {
-  register int slot;
+  for (int slot = 0; slot < NPROCS; slot++)
-
-  for (slot = 0; slot < NPROCS; slot++)
     if (UJ[slot].PID == pid) {
       DBPRINT(("find_process_slot = %d.\n", slot));
       return slot;
@@ -139,15 +138,15 @@ int find_process_slot(register int pid)
 void wait_for_comm_processes(void) {
   int pid;
   int slot;
-  unsigned char d[5];
+  unsigned char d[6];
 
   memset(d, 0, sizeof(d));
   d[0] = 'W';
-  write(UnixPipeOut, d, 4);
+  write(UnixPipeOut, d, 6);
-  SAFEREAD(UnixPipeIn, d, 4);
+  SAFEREAD(UnixPipeIn, d, 6);
 
-  pid = (d[0] << 8) | d[1];
+  pid = (d[0] << 8) | d[1] | (d[4] << 16) | (d[5] << 24);
-  while ((pid != 0) && (pid != 65535)) {
+  while (pid != 0) {
     slot = find_process_slot(pid);
     /* Ignore processes that we didn't start (shouldn't happen but
        occasionally does) */
@@ -163,10 +162,10 @@ void wait_for_comm_processes(void) {
     /* Look for another stopped process. */
     memset(d, 0, sizeof(d));
     d[0] = 'W';
-    write(UnixPipeOut, d, 4);
+    write(UnixPipeOut, d, 6);
-    SAFEREAD(UnixPipeIn, d, 4);
+    SAFEREAD(UnixPipeIn, d, 6);
 
-    pid = (d[0] << 8) | d[1];
+    pid = (d[0] << 8) | d[1] | (d[4] << 16) | (d[5] << 24);
   }
 }
 
@@ -195,11 +194,11 @@ char *build_socket_pathname(int desc) {
 
 void close_unix_descriptors(void) /* Get ready to shut Maiko down */
 {
-  int slot;
+  for (int slot = 0; slot < NPROCS; slot++) {
-
-  for (slot = 0; slot < NPROCS; slot++) {
     /* If this slot has an active job */
     switch (UJ[slot].type) {
+      case UJUNUSED:
+        break;
       case UJSHELL:
         if (kill(UJ[slot].PID, SIGKILL) < 0) perror("Killing shell");
         UJ[slot].PID = 0;
@@ -248,7 +247,6 @@ void close_unix_descriptors(void) /* Get ready to shut Maiko down */
 
 int FindUnixPipes(void) {
   char *envtmp;
-  register int i;
   struct unixjob cleareduj;
 
   DBPRINT(("Entering FindUnixPipes\n"));
@@ -267,7 +265,7 @@ int FindUnixPipes(void) {
   cleareduj.PID = 0;
   cleareduj.readsock = 0;
   cleareduj.type = UJUNUSED;
-  for (i = 0; i < NPROCS; i++) UJ[i] = cleareduj;
+  for (int i = 0; i < NPROCS; i++) UJ[i] = cleareduj;
 
   DBPRINT(("NPROCS is %d; leaving FindUnixPipes\n", NPROCS));
   return (UnixPipeIn == -1 || UnixPipeOut == -1 || StartTime == -1 || UnixPID == -1);
@@ -347,7 +345,7 @@ int FindAvailablePty(char *Master, char *Slave) {
 
 LispPTR Unix_handlecomm(LispPTR *args) {
   int command, dest, i, slot, sock;
-  unsigned char d[4];
+  unsigned char d[6];
   unsigned char ch;
   unsigned char buf[1];
 
@@ -388,13 +386,13 @@ LispPTR Unix_handlecomm(LispPTR *args) {
       memset(d, 0, sizeof(d));
       d[0] = 'F';
       d[3] = sockFD;
-      write(UnixPipeOut, d, 4);
+      write(UnixPipeOut, d, 6);
       WriteLispStringToPipe(args[1]);
 
       DBPRINT(("Sending cmd string: %s\n", shcom));
 
       /* Get status */
-      SAFEREAD(UnixPipeIn, d, 4);
+      SAFEREAD(UnixPipeIn, d, 6);
 
       /* If it worked, return job # */
       if (d[3] == 1) {
@@ -413,7 +411,7 @@ LispPTR Unix_handlecomm(LispPTR *args) {
         }
         UJ[PipeFD].type = UJPROCESS;
         UJ[PipeFD].status = -1;
-        UJ[PipeFD].PID = (d[1] << 8) | d[2];
+        UJ[PipeFD].PID = (d[1] << 8) | d[2] | (d[4] << 16) | (d[5] << 24);
         UJ[PipeFD].readsock = 0;
         close(sockFD);
         unlink(PipeName);
@@ -509,10 +507,9 @@ LispPTR Unix_handlecomm(LispPTR *args) {
           case UJPROCESS:
             /* First check to see it hasn't already died */
             if (UJ[slot].status == -1) {
-              int i;
               /* Kill the job */
               kill(UJ[slot].PID, SIGKILL);
-              for (i = 0; i < 10; i++) {
+              for (int i = 0; i < 10; i++) {
                 /* Waiting for the process to exit is possibly risky.
                    Sending SIGKILL is always supposed to kill
                    a process, but on very rare occurrences this doesn't
@@ -525,11 +522,15 @@ LispPTR Unix_handlecomm(LispPTR *args) {
               }
             }
             break;
-        }
+          default: break;
+          }
       else
         return (ATOM_T);
 
       switch (UJ[slot].type) {
+        case UJUNUSED:
+          break;
+
         case UJSHELL:
           DBPRINT(("Kill 3 closing shell desc %d.\n", slot));
           close(slot);
@@ -585,7 +586,9 @@ LispPTR Unix_handlecomm(LispPTR *args) {
       d[1] = SlavePTY[0];
       d[2] = SlavePTY[1];
       d[3] = slot;
-      write(UnixPipeOut, d, 4);
+      d[4] = '\0';
+      d[5] = '\0';
+      write(UnixPipeOut, d, 6);
 
       len = strlen(SlavePTY) + 1;
       write(UnixPipeOut, &len, 2);
@@ -597,7 +600,7 @@ LispPTR Unix_handlecomm(LispPTR *args) {
       }
 
       /* Get status */
-      SAFEREAD(UnixPipeIn, d, 4);
+      SAFEREAD(UnixPipeIn, d, 6);
 
       /* If successful, return job # */
       DBPRINT(("Pipe/fork result = %d.\n", d[3]));
@@ -606,7 +609,7 @@ LispPTR Unix_handlecomm(LispPTR *args) {
         fcntl(Master, F_SETFL, fcntl(Master, F_GETFL, 0) | O_NONBLOCK);
 
         UJ[slot].type = UJSHELL; /* so we can find them */
-        UJ[slot].PID = (d[1] << 8) | d[2];
+        UJ[slot].PID = (d[1] << 8) | d[2] | (d[4] << 16) | (d[5] << 24);
         printf("Shell job %d, PID = %d\n", slot, UJ[slot].PID);
         UJ[slot].status = -1;
         DBPRINT(("Forked pty in slot %d.\n", slot));
@@ -614,7 +617,7 @@ LispPTR Unix_handlecomm(LispPTR *args) {
       } else {
         printf("Fork failed.\n");
         fflush(stdout);
-        printf("d = %d, %d, %d, %d\n", d[0], d[1], d[2], d[3]);
+        printf("d = %d, %d, %d, %d, %d, %d\n", d[0], d[1], d[2], d[3], d[4], d[5]);
         close(Master);
         return (NIL);
       }
@@ -632,12 +635,15 @@ LispPTR Unix_handlecomm(LispPTR *args) {
       d[1] = dest;
 
       d[3] = 1;
-      write(UnixPipeOut, d, 4);
+      write(UnixPipeOut, d, 6);
 
       /* Get status */
-      SAFEREAD(UnixPipeIn, d, 4);
+      SAFEREAD(UnixPipeIn, d, 6);
 
       switch (UJ[dest].type) {
+        case UJUNUSED:
+          break;
+
         case UJSHELL:
           DBPRINT(("Kill 5 closing shell desc %d.\n", dest));
           close(dest);
@@ -888,6 +894,10 @@ LispPTR Unix_handlecomm(LispPTR *args) {
           /* Either way, signal EOF. */
           DBPRINT(("Indicating write failure from PTY desc %d.\n", slot));
           return (NIL);
+
+        case UJUNUSED:
+        case UJSOCKET:
+          return (NIL);
       }
     }
 

src/unixfork.c

@@ -54,16 +54,14 @@ char shcom[512]; /* Here because I'm suspicious of */
                  /* large allocations on the stack */
 
 
-static __inline__ ssize_t
+static inline ssize_t SAFEREAD(int f, char *b, int c)
-SAFEREAD(int f, char *b, int c)
 {
   ssize_t res;
-loop:
+  do {
-  res = read(f, b, c);
+    res = read(f, b, c);
-  if ((res < 0)) {
+    if (res >= 0) return (res);
-    if (errno == EINTR || errno == EAGAIN) goto loop;
+  } while (errno == EINTR || errno == EAGAIN);
-    perror("reading UnixPipeIn");
+  perror("reading UnixPipeIn");
-  }
   return (res);
 }
 
@@ -79,7 +77,7 @@ loop:
 
 /* Creates a PTY connection to a csh */
 
-int ForkUnixShell(int slot, char *PtySlave, char *termtype, char *shellarg)
+static int ForkUnixShell(int slot, char *PtySlave, char *termtype, char *shellarg)
 {
   int PID, SlaveFD;
   struct termios tio;
@@ -152,7 +150,7 @@ int ForkUnixShell(int slot, char *PtySlave, char *termtype, char *shellarg)
 /* fork_Unix is the secondary process spawned right after LISP is
    started, to avoid having TWO 8 mbyte images sitting around. It listens
    to the pipe LispToUnix waiting for requests, and responds on UnixToLisp.
-   The data passed through this pipe is in 4 byte packets, of the form:
+   The data passed through this pipe is in 6 byte packets, of the form:
 
    Byte 0:   Command character, one of:
                    S: Fork PTY (shell) process. This is used for CHAT windows.
@@ -169,26 +167,26 @@ int ForkUnixShell(int slot, char *PtySlave, char *termtype, char *shellarg)
              Only used for W command, contains byte to write
              [For S&P, pty number]
    Byte 3:   Slot number.
+   Byte 4:   unused.
+   Byte 5:   unused.
+ 
 
-In the case of F & P commands, additional data follows the 4 byte packet.
+In the case of F & P commands, additional data follows the 6 byte packet.
 This consists of 2 bytes representing the length of the shell command
 string, and the string itself.
 
-fork_Unix will return another 4 byte packet. The bytes are the same as those
+fork_Unix will return another 6 byte packet. The bytes are the same as those
 of the packet received except:
 
-   F:        Byte 2 is job number
+   F, S, P:  Bytes 1, 2, 4, and 5 are the Unix process id
-             Byte 3 is 1 if successful, 0 if not
-   S:	     Byte 2 is job number
              Byte 3 is 1 if successful, 0 if not
    R:        Byte 2 is value of byte read from stdin, if any
              Byte 3 is 1 if successful, 2 if EOF, 0 if nothing waiting
-   W:        Bytes 0 & 1 are the Process ID of the terminated process
+   W:        Bytes 0, 1, 4, 5 are the Process ID of the terminated process
              Bytes 2 & 3 are the high & low bytes of the exit status.
    K:        Bytes 1 and 2 are the high and low bytes of the exit status
              of the process.
              Byte 3 is 1 if an exit status was available.
-   E:        Always the same
    C:        Always the same
 */
 
@@ -199,7 +197,7 @@ int fork_Unix() {
   pid_t pid;
   sigset_t signals;
 
-  char IOBuf[4];
+  char IOBuf[6]; /* XXX: signed, because slot comes from here */
   unsigned short tmp = 0;
   char *cmdstring;
 
@@ -263,17 +261,13 @@ int fork_Unix() {
 
   while (1) {
     ssize_t len;
-    len = 0;
+    len = SAFEREAD(LispPipeIn, IOBuf, 6);
-    while (len != 4) {
+    if (len < 0) {
-      if ((len = SAFEREAD(LispPipeIn, IOBuf, 4)) < 0) { /* Get packet */
+      perror("Error reading packet by slave");
-        perror("Packet read by slave");
+      exit(0);
-        /*      kill_comm_processes(); */
+    } else if (len != 6) {
-        exit(0);
+      DBPRINT(("Input packet wrong length: %d", len));
-      }
+      exit(0);
-      if (len != 4) {
-        DBPRINT(("Input packet wrong length:  %d.\n", len));
-        exit(0);
-      }
     }
     slot = IOBuf[3];
     IOBuf[3] = 1; /* Start by signalling success in return-code */
@@ -318,6 +312,8 @@ int fork_Unix() {
             /* ForkUnixShell sets the pid and standard in/out variables */
             IOBuf[1] = (pid >> 8) & 0xFF;
             IOBuf[2] = pid & 0xFF;
+            IOBuf[4] = (pid >> 16) & 0xFF;
+            IOBuf[5] = (pid >> 24) & 0xFF;
           }
         } else {
           printf("Can't get process slot for PTY shell.\n");
@@ -388,6 +384,8 @@ int fork_Unix() {
           } else {
             IOBuf[1] = (pid >> 8) & 0xFF;
             IOBuf[2] = pid & 0xFF;
+            IOBuf[4] = (pid >> 16) & 0xFF;
+            IOBuf[5] = (pid >> 24) & 0xFF;
           }
         } else {
           printf("No process slots available.\n");
@@ -411,6 +409,8 @@ int fork_Unix() {
           /* Ignore processes which are suspended but haven't exited
              (this shouldn't happen) */
           if (WIFSTOPPED(status)) break;
+          IOBuf[5] = (pid >> 24) & 0xFF;
+          IOBuf[4] = (pid >> 16) & 0xFF;
           IOBuf[3] = status >> 8;
           IOBuf[2] = status & 0xFF;
           IOBuf[1] = pid & 0xFF;
@@ -427,7 +427,7 @@ int fork_Unix() {
     } /* End of switch */
 
     /* Return the status/data packet */
-    write(LispPipeOut, IOBuf, 4);
+    write(LispPipeOut, IOBuf, 6);
   }
 }