CGIServlet does not kill CGI child process on IOException, nor terminate child
after a timeout.

We had a problem where a CGI child process (in perl, on windows) remained
running for a long time. In some situations where the client disconnected, an
IOException was thrown in the servlet, but this did not result in a cleanup of
the CGI child process. This was a problem as our CGI program accessed a single
threaded resource over TCP, and could therefore lock the system up indefinitly.

===

--- CGIServlet.java	Sun May 08 04:46:05 2005
+++
C:\Temp\hjakarta-tomcat-5.5.9-src.tar\jakarta-tomcat-5.5.9-src\jakarta-tomcat-catalina\catalina\src\share\org\apache\catalina\servlets\CGIServlet.java
Sat Mar 26 18:24:02 2005
@@ -267,8 +267,6 @@
     private String parameterEncoding = System.getProperty("file.encoding",
                                                           "UTF-8");
 
-    private long lScriptTimeoutMillis = 2000;
-
     /** object used to ensure multiple threads don't try to expand same file */
     static Object expandFileLock = new Object();
 
@@ -332,16 +330,10 @@
             cgiExecutable = value;
         }
 
-
         value = getServletConfig().getInitParameter("parameterEncoding");
         if (value != null) {
             parameterEncoding = value;
         }
-        // Added by CJD
-        value = getServletConfig().getInitParameter("scriptTimeout");
-        if (value != null) {
-            lScriptTimeoutMillis = Integer.valueOf(value).intValue();
-        }
 
         // Identify the internal container resources we need
         context = config.getServletContext();
@@ -1440,7 +1432,7 @@
      */
 
     protected class CGIRunner {
-        private Process proc = null;
+
         /** script/command to be executed */
         private String command = null;
 
@@ -1656,6 +1648,7 @@
             InputStream cgiOutput = null;
             BufferedReader commandsStdErr = null;
             BufferedOutputStream commandsStdIn = null;
+            Process proc = null;
             int bufRead = -1;
 
             //create query arguments
@@ -1731,200 +1724,138 @@
             }
 
             rt = Runtime.getRuntime();
-            try {
+            proc = rt.exec(cmdAndArgs.toString(), hashToStringArray(env), wd);
 
-                proc = rt.exec(cmdAndArgs.toString(), hashToStringArray(env), wd);
+            if(contentStream != null) {
+                commandsStdIn = new BufferedOutputStream(proc.getOutputStream());
+                commandsStdIn.write(contentStream.toByteArray());
+                commandsStdIn.flush();
+                commandsStdIn.close();
+            }
 
-                if(contentStream != null) {
-                    commandsStdIn = new
BufferedOutputStream(proc.getOutputStream());
-                    commandsStdIn.write(contentStream.toByteArray());
-                    commandsStdIn.flush();
-                    commandsStdIn.close();
-                }
+            /* we want to wait for the process to exit,  Process.waitFor()
+             * is useless in our situation; see
+             * http://developer.java.sun.com/developer/
+             *                               bugParade/bugs/4223650.html
+             */
 
-                /* we want to wait for the process to exit,  Process.waitFor()
-                 * is useless in our situation; see
-                 * http://developer.java.sun.com/developer/
-                 *                               bugParade/bugs/4223650.html
-                 */
-
-                boolean isRunning = true;
-                commandsStdErr = new BufferedReader
-                    (new InputStreamReader(proc.getErrorStream()));
-                BufferedWriter servletContainerStdout = null;
+            boolean isRunning = true;
+            commandsStdErr = new BufferedReader
+                (new InputStreamReader(proc.getErrorStream()));
+            BufferedWriter servletContainerStdout = null;
 
-                try {
-                    if (response.getOutputStream() != null) {
-                        servletContainerStdout =
-                            new BufferedWriter(new OutputStreamWriter
-                                (response.getOutputStream()));
-                    }
-                } catch (IOException ignored) {
-                    //NOOP: no output will be written
+            try {
+                if (response.getOutputStream() != null) {
+                    servletContainerStdout =
+                        new BufferedWriter(new OutputStreamWriter
+                            (response.getOutputStream()));
                 }
-                final BufferedReader stdErrRdr = commandsStdErr ;
+            } catch (IOException ignored) {
+                //NOOP: no output will be written
+            }
+            final BufferedReader stdErrRdr = commandsStdErr ;
 
-                new Thread() {
-                        public void run () {
-                            sendToLog(stdErrRdr) ;
-                        } ;
-                    }.start() ;
-
-
-                new Thread() {
-                        public void run () {
-                            deadProcWatcher(proc,lScriptTimeoutMillis) ;
-                        } ;
-                    }.start() ;
-
-                InputStream cgiHeaderStream =
-                    new HTTPHeaderInputStream(proc.getInputStream());
-                BufferedReader cgiHeaderReader =
-                    new BufferedReader(new InputStreamReader(cgiHeaderStream));
-                boolean isBinaryContent = false;
+            new Thread() {
+                public void run () {
+                    sendToLog(stdErrRdr) ;
+                } ;
+            }.start() ;
+
+            InputStream cgiHeaderStream =
+                new HTTPHeaderInputStream(proc.getInputStream());
+            BufferedReader cgiHeaderReader =
+                new BufferedReader(new InputStreamReader(cgiHeaderStream));
+            boolean isBinaryContent = false;
             
-
-
-                while (isRunning) {
-                    try {
-                        //set headers
-                        String line = null;
-                        while (((line = cgiHeaderReader.readLine()) != null)
-                               && !("".equals(line))) {
-                            if (debug >= 2) {
-                                log("runCGI: addHeader(\"" + line + "\")");
-                            }
-                            if (line.startsWith("HTTP")) {
-                                //TODO: should set status codes (NPH support)
-                                /*
-                                 * response.setStatus(getStatusCode(line));
-                                 */
-                            } else if (line.indexOf(":") >= 0) {
-                                String header =
-                                    line.substring(0, line.indexOf(":")).trim();
-                                String value =
-                                    line.substring(line.indexOf(":") + 1).trim(); 
-                                response.addHeader(header , value);
-                                if ((header.toLowerCase().equals("content-type"))
-                                    && (!value.toLowerCase().startsWith("text"))) {
-                                    isBinaryContent = true;
-                                }
-                            } else {
-                                log("runCGI: bad header line \"" + line + "\"");
+            while (isRunning) {
+                try {
+                    //set headers
+                    String line = null;
+                    while (((line = cgiHeaderReader.readLine()) != null)
+                           && !("".equals(line))) {
+                        if (debug >= 2) {
+                            log("runCGI: addHeader(\"" + line + "\")");
+                        }
+                        if (line.startsWith("HTTP")) {
+                            //TODO: should set status codes (NPH support)
+                            /*
+                             * response.setStatus(getStatusCode(line));
+                             */
+                        } else if (line.indexOf(":") >= 0) {
+                            String header =
+                                line.substring(0, line.indexOf(":")).trim();
+                            String value =
+                                line.substring(line.indexOf(":") + 1).trim(); 
+                            response.addHeader(header , value);
+                            if ((header.toLowerCase().equals("content-type"))
+                                && (!value.toLowerCase().startsWith("text"))) {
+                                isBinaryContent = true;
                             }
+                        } else {
+                            log("runCGI: bad header line \"" + line + "\"");
                         }
+                    }
 
-                        //write output
-                        if (isBinaryContent) {
-                            byte[] bBuf = new byte[2048];
-                            OutputStream out = response.getOutputStream();
-                            cgiOutput = proc.getInputStream();
-                            while ((bufRead = cgiOutput.read(bBuf)) != -1) {
-                                if (debug >= 4) {
-                                    log("runCGI: output " + bufRead +
-                                        " bytes of binary data");
-                                }
-                                out.write(bBuf, 0, bufRead);
+                    //write output
+                    if (isBinaryContent) {
+                        byte[] bBuf = new byte[2048];
+                        OutputStream out = response.getOutputStream();
+                        cgiOutput = proc.getInputStream();
+                        while ((bufRead = cgiOutput.read(bBuf)) != -1) {
+                            if (debug >= 4) {
+                                log("runCGI: output " + bufRead +
+                                    " bytes of binary data");
                             }
-                        } else {
-                            commandsStdOut = new BufferedReader
-                                (new InputStreamReader(proc.getInputStream()));
+                            out.write(bBuf, 0, bufRead);
+                        }
+                    } else {
+                        commandsStdOut = new BufferedReader
+                            (new InputStreamReader(proc.getInputStream()));
 
-                            char[] cBuf = new char[1024];
-                            try {
-                                while ((bufRead = commandsStdOut.read(cBuf)) !=
-1) {
-                                    if (servletContainerStdout != null) {
-                                        if (debug >= 4) {
-                                            log("runCGI: write(\"" +
+                        char[] cBuf = new char[1024];
+                        try {
+                            while ((bufRead = commandsStdOut.read(cBuf)) != -1) {
+                                if (servletContainerStdout != null) {
+                                    if (debug >= 4) {
+                                        log("runCGI: write(\"" +
                                                 new String(cBuf, 0, bufRead) +
"\")");
-                                        }
-                                        servletContainerStdout.write(cBuf, 0,
bufRead);
                                     }
-                                }
-                            } finally {
-                                // Attempt to consume any leftover byte if
something bad happens,
-                                // such as a socket disconnect on the servlet
side; otherwise, the
-                                // external process could hang
-                                if (bufRead != -1) {
-                                    while ((bufRead =
commandsStdOut.read(cBuf)) != -1) {}
+                                    servletContainerStdout.write(cBuf, 0, bufRead);
                                 }
                             }
-    
-                            if (servletContainerStdout != null) {
-                                servletContainerStdout.flush();
+                        } finally {
+                            // Attempt to consume any leftover byte if
something bad happens,
+                            // such as a socket disconnect on the servlet side;
otherwise, the
+                            // external process could hang
+                            if (bufRead != -1) {
+                                while ((bufRead = commandsStdOut.read(cBuf)) !=
-1) {}
                             }
                         }
-
-                        proc.exitValue(); // Throws exception if alive
-
-                        isRunning = false;
-
-                    } catch (IllegalThreadStateException e) {
-                        try {
-                            Thread.sleep(500);
-                        } catch (InterruptedException ignored) {
+    
+                        if (servletContainerStdout != null) {
+                            servletContainerStdout.flush();
                         }
                     }
-               
-                } //replacement for Process.waitFor()
-                // Close the output stream used
-                if (isBinaryContent) {
-                    cgiOutput.close();
-                } else {
-                    commandsStdOut.close();
-                }
-            }
-            catch (IOException e){
-                log ("Caught exception " + e);
-                System.out.println("Caught IOException - printing dump 1");
-                e.printStackTrace();
-                if (proc != null){
-                    proc.destroy();
-                    proc = null;
-                }
-                throw new IOException (e.toString());
-            }
-            finally{
-                log ("Running finally block");
-                if (proc != null){
-                    proc.destroy();
-                    proc = null;
-                }
-            }
-        }
 
-        private void deadProcWatcher(Process myproc,long lWait){
-            long processRunDeadline = System.currentTimeMillis() +lWait;
+                    proc.exitValue(); // Throws exception if alive
+
+                    isRunning = false;
 
-            while(System.currentTimeMillis() < processRunDeadline){
-                log("deadProcWatcher: cgi run:  " + 
-                    processRunDeadline + " " +
-                    System.currentTimeMillis() +
-                    " " + myproc);
-                try{
-                    myproc.exitValue(); // Throws exception if alive
-                    log("deadProcWatcher: exiting normally");
-                    return; // child has exited, we can exit
-                    
                 } catch (IllegalThreadStateException e) {
                     try {
                         Thread.sleep(500);
                     } catch (InterruptedException ignored) {
                     }
                 }
+            } //replacement for Process.waitFor()
+            // Close the output stream used
+            if (isBinaryContent) {
+                cgiOutput.close();
+            } else {
+                commandsStdOut.close();
             }
-            if (System.currentTimeMillis() > processRunDeadline){
-                log("Killing process due to timeout" + 
-                    processRunDeadline + " " +
-                    System.currentTimeMillis() +
-                    " " + myproc);
-                myproc.destroy();
-            }
-            
         }
 
-        
-        
         private void sendToLog(BufferedReader rdr) {
             String line = null;
             int lineCount = 0 ;