Re: svn commit: r1539988 - /httpd/httpd/trunk/server/log.c

2013-11-20 Thread Jeff Trawick 
On Mon, Nov 18, 2013 at 10:45 AM, Jeff Trawick traw...@gmail.com wrote:

 On Fri, Nov 8, 2013 at 6:41 AM, jkal...@apache.org wrote:

 Author: jkaluza
 Date: Fri Nov  8 11:41:08 2013
 New Revision: 1539988

 URL: http://svn.apache.org/r1539988
 Log:
 Do not lose log messages with NULL server_rec when error log provider is
 used.

 - set stderr_log to NULL after it is redirected to /dev/null
 - use log provider of ap_server_conf in log_error_core when no server_rec
   is provided and std_err_log is NULL


 I think this is resulting in a crash in my provider when ap_log_error() is
 called before open-logs (i.e., before my provider has had a chance to set
 up a destination for logging).

 This is the original 2.4 expectation regarding NULL server_rec IIUC: NULL
 is okay until logs are opened, at which point a NULL server_rec is a bug in
 the caller.  But just pass ap_server_conf when you don't have a
 context-specific server_rec and core httpd will make sure it is always set
 when logging requires a server_rec.

 If at all practical, a NULL server_rec prior to setting up a configured
 provider shouldn't be treated differently than a NULL server_rec prior to
 setting a configured error log file/piped logger.

 My provider is getting a call on this path:

 #1  0x71531a7b in elprov_error_log (info=0x7fffb340,
 handle=0x0,
 errstr=0x7fffb390 AH00558: httpd: Could not reliably determine
 the server's fully qualified domain name, using 127.0.1.1. Set the
 'ServerName' directive globally to suppress this message\n,
 len=169) at mod_elprov.c:38
 #2  0x0046a3f9 in log_error_core (file=0x488f00 util.c,
 line=2201, module_index=0,
 level=65, status=0, s=0x0, c=0x0, r=0x0, pool=0x6b6138,
 fmt=0x489618 AH00558: %s: Could not reliably determine the server's
 fully qualified domain name, using %s. Set the 'ServerName' directive
 globally to suppress this message, args=0x7fffd408)
 at log.c:1259
 #3  0x0046a709 in ap_log_perror_ (file=0x488f00 util.c,
 line=2201, module_index=0,
 level=65, status=0, p=0x6b6138,
 fmt=0x489618 AH00558: %s: Could not reliably determine the server's
 fully qualified domain name, using %s. Set the 'ServerName' directive
 globally to suppress this message) at log.c:1311
 #4  0x004335e7 in ap_get_local_host (a=0x6b6138) at util.c:2201
 #5  0x0042d9f9 in ap_fini_vhost_config (p=0x6b6138,
 main_s=0x6df320) at vhost.c:565
 #6  0x0042cac5 in main (argc=2, argv=0x7fffe0e8) at main.c:758

 Maybe the handle should be checked (!= NULL) to see if there is a provider
 AND we've passed the critical point in the open-logs hook where the
 provider becomes usable.  The call to the provider's init function in
 open-logs will abort startup if NULL is returned for the handle, so handle
 will never be NULL if the provider is initialized.


I guess this is what really happened:

config was processed the first time, and open-logs was called the first
time, and stderr was dropped

config has been processed the second time but open-logs hasn't been called
again yet

no stderr, provider in server_rec but no provider handle; I guess I just
need to tweak your if statement that checks for no available destination

r1543979




 Modified:
 httpd/httpd/trunk/server/log.c

 Modified: httpd/httpd/trunk/server/log.c
 URL:
 http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=1539988r1=1539987r2=1539988view=diff

 ==
 --- httpd/httpd/trunk/server/log.c (original)
 +++ httpd/httpd/trunk/server/log.c Fri Nov  8 11:41:08 2013
 @@ -437,9 +437,12 @@ int ap_open_logs(apr_pool_t *pconf, apr_
  #define NULL_DEVICE /dev/null
  #endif

 -if (replace_stderr  freopen(NULL_DEVICE, w, stderr) == NULL) {
 -ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main,
 APLOGNO(00093)
 - unable to replace stderr with %s, NULL_DEVICE);
 +if (replace_stderr) {
 +if (freopen(NULL_DEVICE, w, stderr) == NULL) {
 +ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main,
 APLOGNO(00093)
 +unable to replace stderr with %s, NULL_DEVICE);
 +}
 +stderr_log = NULL;
  }

  for (virt = s_main-next; virt; virt = virt-next) {
 @@ -1034,6 +1037,8 @@ static void log_error_core(const char *f
  const request_rec *rmain = NULL;
  core_server_config *sconf = NULL;
  ap_errorlog_info info;
 +ap_errorlog_provider *errorlog_provider = NULL;
 +void *errorlog_provider_handle = NULL;

  /* do we need to log once-per-req or once-per-conn info? */
  int log_conn_info = 0, log_req_info = 0;
 @@ -1060,6 +1065,10 @@ static void log_error_core(const char *f
  #endif

  logf = stderr_log;
 +if (!logf  ap_server_conf 
 ap_server_conf-errorlog_provider) {
 +errorlog_provider = ap_server_conf-errorlog_provider;
 +errorlog_provider_handle =

Re: svn commit: r1539988 - /httpd/httpd/trunk/server/log.c

2013-11-18 Thread Jeff Trawick 
On Fri, Nov 8, 2013 at 6:41 AM, jkal...@apache.org wrote:

 Author: jkaluza
 Date: Fri Nov  8 11:41:08 2013
 New Revision: 1539988

 URL: http://svn.apache.org/r1539988
 Log:
 Do not lose log messages with NULL server_rec when error log provider is
 used.

 - set stderr_log to NULL after it is redirected to /dev/null
 - use log provider of ap_server_conf in log_error_core when no server_rec
   is provided and std_err_log is NULL


I think this is resulting in a crash in my provider when ap_log_error() is
called before open-logs (i.e., before my provider has had a chance to set
up a destination for logging).

This is the original 2.4 expectation regarding NULL server_rec IIUC: NULL
is okay until logs are opened, at which point a NULL server_rec is a bug in
the caller.  But just pass ap_server_conf when you don't have a
context-specific server_rec and core httpd will make sure it is always set
when logging requires a server_rec.

If at all practical, a NULL server_rec prior to setting up a configured
provider shouldn't be treated differently than a NULL server_rec prior to
setting a configured error log file/piped logger.

My provider is getting a call on this path:

#1  0x71531a7b in elprov_error_log (info=0x7fffb340,
handle=0x0,
errstr=0x7fffb390 AH00558: httpd: Could not reliably determine the
server's fully qualified domain name, using 127.0.1.1. Set the 'ServerName'
directive globally to suppress this message\n,
len=169) at mod_elprov.c:38
#2  0x0046a3f9 in log_error_core (file=0x488f00 util.c,
line=2201, module_index=0,
level=65, status=0, s=0x0, c=0x0, r=0x0, pool=0x6b6138,
fmt=0x489618 AH00558: %s: Could not reliably determine the server's
fully qualified domain name, using %s. Set the 'ServerName' directive
globally to suppress this message, args=0x7fffd408)
at log.c:1259
#3  0x0046a709 in ap_log_perror_ (file=0x488f00 util.c,
line=2201, module_index=0,
level=65, status=0, p=0x6b6138,
fmt=0x489618 AH00558: %s: Could not reliably determine the server's
fully qualified domain name, using %s. Set the 'ServerName' directive
globally to suppress this message) at log.c:1311
#4  0x004335e7 in ap_get_local_host (a=0x6b6138) at util.c:2201
#5  0x0042d9f9 in ap_fini_vhost_config (p=0x6b6138,
main_s=0x6df320) at vhost.c:565
#6  0x0042cac5 in main (argc=2, argv=0x7fffe0e8) at main.c:758

Maybe the handle should be checked (!= NULL) to see if there is a provider
AND we've passed the critical point in the open-logs hook where the
provider becomes usable.  The call to the provider's init function in
open-logs will abort startup if NULL is returned for the handle, so handle
will never be NULL if the provider is initialized.


 Modified:
 httpd/httpd/trunk/server/log.c

 Modified: httpd/httpd/trunk/server/log.c
 URL:
 http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=1539988r1=1539987r2=1539988view=diff

 ==
 --- httpd/httpd/trunk/server/log.c (original)
 +++ httpd/httpd/trunk/server/log.c Fri Nov  8 11:41:08 2013
 @@ -437,9 +437,12 @@ int ap_open_logs(apr_pool_t *pconf, apr_
  #define NULL_DEVICE /dev/null
  #endif

 -if (replace_stderr  freopen(NULL_DEVICE, w, stderr) == NULL) {
 -ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main, APLOGNO(00093)
 - unable to replace stderr with %s, NULL_DEVICE);
 +if (replace_stderr) {
 +if (freopen(NULL_DEVICE, w, stderr) == NULL) {
 +ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main,
 APLOGNO(00093)
 +unable to replace stderr with %s, NULL_DEVICE);
 +}
 +stderr_log = NULL;
  }

  for (virt = s_main-next; virt; virt = virt-next) {
 @@ -1034,6 +1037,8 @@ static void log_error_core(const char *f
  const request_rec *rmain = NULL;
  core_server_config *sconf = NULL;
  ap_errorlog_info info;
 +ap_errorlog_provider *errorlog_provider = NULL;
 +void *errorlog_provider_handle = NULL;

  /* do we need to log once-per-req or once-per-conn info? */
  int log_conn_info = 0, log_req_info = 0;
 @@ -1060,6 +1065,10 @@ static void log_error_core(const char *f
  #endif

  logf = stderr_log;
 +if (!logf  ap_server_conf  ap_server_conf-errorlog_provider)
 {
 +errorlog_provider = ap_server_conf-errorlog_provider;
 +errorlog_provider_handle =
 ap_server_conf-errorlog_provider_handle;
 +}
  }
  else {
  int configured_level = r ? ap_get_request_module_loglevel(r,
 module_index):
 @@ -1078,6 +1087,9 @@ static void log_error_core(const char *f
  logf = s-error_log;
  }

 +errorlog_provider = s-errorlog_provider;
 +errorlog_provider_handle = s-errorlog_provider_handle;
 +
  /* the faked server_rec from mod_cgid does not have
 s-module_config */
  if (s-module_config)