svn commit: r503340 - /httpd/httpd/trunk/modules/generators/mod_cgid.c

[tr...@apache.org]

Author: trawick
Date: Sat Feb  3 16:57:03 2007
New Revision: 503340

URL: http://svn.apache.org/viewvc?view=rev&rev=503340
Log:
Update the hash table of active script pids even on paths where a 
script process wasn't created (storing 0 for the pid in that case).
Otherwise, the remembered pid is that of the last successful script
execution for this hash key.

Prior to this patch, the wrong process could be terminated in
rare circumstances:

- A CGI process with pid 10101 is forked for connection 99.

- After the CGI exits and some time elapses, some other process gets 
  pid 10101. (Connection 99 hasn't handled another CGI request yet.)

- The next time connection 99 has a CGI process, the fork()
  or other early setup fails, so no CGI process is created.

- The remembered pid for connection 99 is still 10101.  It
  gets terminated (subject to permissions).

Modified:
    httpd/httpd/trunk/modules/generators/mod_cgid.c

Modified: httpd/httpd/trunk/modules/generators/mod_cgid.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/generators/mod_cgid.c?view=diff&rev=503340&r1=503339&r2=503340
==============================================================================
--- httpd/httpd/trunk/modules/generators/mod_cgid.c (original)
+++ httpd/httpd/trunk/modules/generators/mod_cgid.c Sat Feb  3 16:57:03 2007
@@ -626,6 +626,7 @@
         apr_file_t *inout;
         cgid_req_t cgid_req;
         apr_status_t stat;
+        void *key;
 
         apr_pool_clear(ptrans);
 
@@ -718,6 +719,8 @@
              */
             ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
                          "couldn't set child process attributes: %s", r->filename);
+
+            procnew->pid = 0; /* no process to clean up */
         }
         else {
             apr_pool_userdata_set(r, ERRFN_USERDATA_KEY, apr_pool_cleanup_null, ptrans);
@@ -754,31 +757,35 @@
                 ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
                              "couldn't create child process: %d: %s", rc,
                              apr_filepath_name_get(r->filename));
-            }
-            else {
-                /* We don't want to leak storage for the key, so only allocate
-                 * a key if the key doesn't exist yet in the hash; there are
-                 * only a limited number of possible keys (one for each
-                 * possible thread in the server), so we can allocate a copy
-                 * of the key the first time a thread has a cgid request.
-                 * Note that apr_hash_set() only uses the storage passed in
-                 * for the key if it is adding the key to the hash for the
-                 * first time; new key storage isn't needed for replacing the
-                 * existing value of a key.
-                 */
-                void *key;
 
-                if (apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id))) {
-                    key = &cgid_req.conn_id;
-                }
-                else {
-                    key = apr_pcalloc(pcgi, sizeof(cgid_req.conn_id));
-                    memcpy(key, &cgid_req.conn_id, sizeof(cgid_req.conn_id));
-                }
-                apr_hash_set(script_hash, key, sizeof(cgid_req.conn_id),
-                             (void *)((long)procnew->pid));
+                procnew->pid = 0; /* no process to clean up */
             }
         }
+
+        /* If the script process was created, remember the pid for
+         * later cleanup.  If the script process wasn't created, clear
+         * out any prior pid with the same key.
+         *
+         * We don't want to leak storage for the key, so only allocate
+         * a key if the key doesn't exist yet in the hash; there are
+         * only a limited number of possible keys (one for each
+         * possible thread in the server), so we can allocate a copy
+         * of the key the first time a thread has a cgid request.
+         * Note that apr_hash_set() only uses the storage passed in
+         * for the key if it is adding the key to the hash for the
+         * first time; new key storage isn't needed for replacing the
+         * existing value of a key.
+         */
+
+        if (apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id))) {
+            key = &cgid_req.conn_id;
+        }
+        else {
+            key = apr_pcalloc(pcgi, sizeof(cgid_req.conn_id));
+            memcpy(key, &cgid_req.conn_id, sizeof(cgid_req.conn_id));
+        }
+        apr_hash_set(script_hash, key, sizeof(cgid_req.conn_id),
+                     (void *)((long)procnew->pid));
     }
     return -1;
 }