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 0x00007ffff1531a7b in elprov_error_log (info=0x7fffffffb340, > handle=0x0, > errstr=0x7fffffffb390 "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 0x000000000046a3f9 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=0x7fffffffd408) > at log.c:1259 > #3 0x000000000046a709 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 0x00000000004335e7 in ap_get_local_host (a=0x6b6138) at util.c:2201 > #5 0x000000000042d9f9 in ap_fini_vhost_config (p=0x6b6138, > main_s=0x6df320) at vhost.c:565 > #6 0x000000000042cac5 in main (argc=2, argv=0x7fffffffe0e8) 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=1539988&r1=1539987&r2=1539988&view=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) { >> sconf = ap_get_core_module_config(s->module_config); >> @@ -1106,6 +1118,14 @@ static void log_error_core(const char *f >> } >> } >> >> + if (!logf && !errorlog_provider) { >> + /* There is no file to send the log message to (or it is >> + * redirected to /dev/null and therefore any formating done below >> + * would be lost anyway) and there is no log provider available, >> so >> + * we just return here. */ >> + return; >> + } >> + >> info.s = s; >> info.c = c; >> info.pool = pool; >> @@ -1191,7 +1211,7 @@ static void log_error_core(const char *f >> continue; >> } >> >> - if (logf || (s->errorlog_provider->flags & >> + if (logf || (errorlog_provider->flags & >> AP_ERRORLOG_PROVIDER_ADD_EOL_STR)) { >> /* Truncate for the terminator (as apr_snprintf does) */ >> if (len > MAX_STRING_LEN - sizeof(APR_EOL_STR)) { >> @@ -1205,8 +1225,8 @@ static void log_error_core(const char *f >> write_logline(errstr, len, logf, level_and_mask); >> } >> else { >> - s->errorlog_provider->writer(&info, >> s->errorlog_provider_handle, >> - errstr, len); >> + errorlog_provider->writer(&info, errorlog_provider_handle, >> + errstr, len); >> } >> >> if (done) { >> >> >> > > > -- > Born in Roswell... married an alien... > http://emptyhammock.com/ > -- Born in Roswell... married an alien... http://emptyhammock.com/
