When starting a new piped error logger for the main server, ensure

that the new child's stderr is not a pipe to an old piped logger:

* server/log.c (log_child): Add "dummy_stderr" parameter; if set,
duplicate stdout as the stderr for the child.
(open_error_log): Add "is_main" parameter; use dummy stderr for logger
for main server only.
(ap_open_logs, ap_open_piped_log): Adjust for new open_error_log()/
log_child() parameters.

PR: 40651
Submitted by: jorton, rpluem

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@452431 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 21c5fc007b9b87f7cc07cacf7c052915721233bb..45cdaab7b0844330db7656713516c655b79bc311 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,10 @@
 Changes with Apache 2.3.0
   [Remove entries to the current 2.0 and 2.2 section below, when backported]
 
+  *) Fix issue which could cause piped loggers to be orphaned and never
+     terminate after a graceful restart.  PR 40651.  [Joe Orton, 
+     Ruediger Pluem]
+
   *) mod_headers: support regexp-based editing of HTTP headers [Nick Kew]
 
   *) mod_echo: Fix precedence problem in if statement. PR 40658.

diff --git a/server/log.c b/server/log.c
index 057672f239477d273be1c397711379feedefdf14..c027f002415f67150fc3a1cd3165d2deb8d8c57b 100644 (file)
--- a/server/log.c
+++ b/server/log.c
@@ -225,8 +225,13 @@ static void log_child_errfn(apr_pool_t *pool, apr_status_t err,
                  "%s", description);
 }
 
+/* Create a child process running PROGNAME with a pipe connected to
+ * the childs stdin.  The write-end of the pipe will be placed in
+ * *FPIN on successful return.  If dummy_stderr is non-zero, the
+ * stderr for the child will be the same as the stdout of the parent.
+ * Otherwise the child will inherit the stderr from the parent. */
 static int log_child(apr_pool_t *p, const char *progname,
-                     apr_file_t **fpin)
+                     apr_file_t **fpin, int dummy_stderr)
 {
     /* Child process code for 'ErrorLog "|..."';
      * may want a common framework for this, since I expect it will
@@ -235,6 +240,7 @@ static int log_child(apr_pool_t *p, const char *progname,
     apr_status_t rc;
     apr_procattr_t *procattr;
     apr_proc_t *procnew;
+    apr_file_t *errfile;
 
     if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
         && ((rc = apr_procattr_cmdtype_set(procattr,
@@ -244,7 +250,12 @@ static int log_child(apr_pool_t *p, const char *progname,
                                       APR_NO_PIPE,
                                       APR_NO_PIPE)) == APR_SUCCESS)
         && ((rc = apr_procattr_error_check_set(procattr, 1)) == APR_SUCCESS)
-        && ((rc = apr_procattr_child_errfn_set(procattr, log_child_errfn)) == APR_SUCCESS)) {
+        && ((rc = apr_procattr_child_errfn_set(procattr, log_child_errfn)) == APR_SUCCESS)
+        && (!dummy_stderr 
+            || ((rc = apr_file_open_stdout(&errfile, p)) == APR_SUCCESS
+                && (rc = apr_procattr_child_err_set(procattr, 
+                                                    errfile, 
+                                                    errfile)) == APR_SUCCESS))) {
         char **args;
         const char *pname;
 
@@ -261,12 +272,21 @@ static int log_child(apr_pool_t *p, const char *progname,
              * close_handle_in_child()
              */
         }
+
+        /* apr_procattr_child_err_set dups errfile twice: close the
+         * remaining "parent-side" copy (apr_proc_create closes the
+         * other). */
+        if (dummy_stderr) {
+            apr_file_close(procnew->err);
+        }
     }
 
     return rc;
 }
 
-static int open_error_log(server_rec *s, apr_pool_t *p)
+/* Open the error log for the given server_rec.  If IS_MAIN is
+ * non-zero, s is the main server. */
+static int open_error_log(server_rec *s, int is_main, apr_pool_t *p)
 {
     const char *fname;
     int rc;
@@ -274,8 +294,11 @@ static int open_error_log(server_rec *s, apr_pool_t *p)
     if (*s->error_fname == '|') {
         apr_file_t *dummy = NULL;
 
-        /* This starts a new process... */
-        rc = log_child (p, s->error_fname + 1, &dummy);
+        /* Spawn a new child logger.  If this is the main server_rec,
+         * the new child must use a dummy stderr since the current
+         * stderr might be a pipe to the old logger.  Otherwise, the
+         * child inherits the parents stderr. */
+        rc = log_child(p, s->error_fname + 1, &dummy, is_main);
         if (rc != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
                          "Couldn't start ErrorLog process");
@@ -338,7 +361,7 @@ int ap_open_logs(apr_pool_t *pconf, apr_pool_t *p /* plog */,
 
     apr_pool_cleanup_register(p, NULL, clear_handle_list,
                               apr_pool_cleanup_null);
-    if (open_error_log(s_main, p) != OK) {
+    if (open_error_log(s_main, 1, p) != OK) {
         return DONE;
     }
 
@@ -377,7 +400,7 @@ int ap_open_logs(apr_pool_t *pconf, apr_pool_t *p /* plog */,
             }
 
             if (q == virt) {
-                if (open_error_log(virt, p) != OK) {
+                if (open_error_log(virt, 0, p) != OK) {
                     return DONE;
                 }
             }
@@ -955,7 +978,7 @@ AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program)
     apr_file_t *dummy = NULL;
     int rc;
 
-    rc = log_child(p, program, &dummy);
+    rc = log_child(p, program, &dummy, 0);
     if (rc != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
                      "Couldn't start piped log process");