You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jk...@apache.org on 2013/11/08 12:41:08 UTC
svn commit: r1539988 - /httpd/httpd/trunk/server/log.c
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
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) {
Re: svn commit: r1539988 - /httpd/httpd/trunk/server/log.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Mon, Nov 18, 2013 at 10:45 AM, Jeff Trawick <tr...@gmail.com> wrote:
> On Fri, Nov 8, 2013 at 6:41 AM, <jk...@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/
Re: svn commit: r1539988 - /httpd/httpd/trunk/server/log.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Nov 8, 2013 at 6:41 AM, <jk...@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.
> 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/