You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Riggs <ji...@riggs.me> on 2018/04/18 12:17:02 UTC
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
I didn't think of this before, but there is one edge case this would miss: if someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It appears that this is already broken in server/log.c, though. Also, log.c is using str(n)casecmp. The following would make things consistent and handle this weird edge case. Thoughts?
Index: server/core.c
===================================================================
--- server/core.c (revision 1829431)
+++ server/core.c (working copy)
@@ -4867,7 +4867,8 @@
static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
{
if (!s->error_fname || s->error_fname[0] == '|'
- || strcmp(s->error_fname, "syslog") == 0) {
+ || strcasecmp(s->error_fname, "syslog") == 0
+ || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
return APR_SUCCESS;
}
else {
@@ -5281,7 +5282,9 @@
apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
tmp = ap_server_root_relative(p, sconf->ap_document_root);
apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
- if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
+ if (s->error_fname[0] != '|'
+ && strcasecmp(s->error_fname, "syslog") != 0
+ && strncasecmp(s->error_fname, "syslog:", 7) != 0)
tmp = ap_server_root_relative(p, s->error_fname);
else
tmp = s->error_fname;
@@ -5456,4 +5459,3 @@
core_cmds, /* command apr_table_t */
register_hooks /* register hooks */
};
-
Index: server/log.c
===================================================================
--- server/log.c (revision 1829431)
+++ server/log.c (working copy)
@@ -396,7 +396,8 @@
}
#ifdef HAVE_SYSLOG
- else if (!strncasecmp(s->error_fname, "syslog", 6)) {
+ else if (strcasecmp(s->error_fname, "syslog") == 0
+ || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
if ((fname = strchr(s->error_fname, ':'))) {
/* s->error_fname could be [level]:[tag] (see #60525) */
const char *tag;
> On 18 Apr 2018, at 04:57, elukey@apache.org wrote:
>
> Author: elukey
> Date: Wed Apr 18 09:57:08 2018
> New Revision: 1829430
>
> URL: http://svn.apache.org/viewvc?rev=1829430&view=rev
> Log:
> core-check_errorlog_dir_syslog.patch: add suggestions from STATUS
>
> Credis to jhriggs and jailletc36
>
>
> Modified:
> httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
>
> Modified: httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
> URL: http://svn.apache.org/viewvc/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch?rev=1829430&r1=1829429&r2=1829430&view=diff
> ==============================================================================
> --- httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch (original)
> +++ httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch Wed Apr 18 09:57:08 2018
> @@ -1,14 +1,23 @@
> Index: server/core.c
> ===================================================================
> ---- server/core.c (revision 1829090)
> +--- server/core.c (revision 1829429)
> +++ server/core.c (working copy)
> @@ -4867,7 +4867,7 @@
> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
> {
> if (!s->error_fname || s->error_fname[0] == '|'
> - || strcmp(s->error_fname, "syslog") == 0) {
> -+ || !strncmp(s->error_fname, "syslog", 6)) {
> ++ || strncmp(s->error_fname, "syslog", 6) == 0) {
> return APR_SUCCESS;
> }
> else {
> +@@ -5281,7 +5281,7 @@
> + apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
> + tmp = ap_server_root_relative(p, sconf->ap_document_root);
> + apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
> +- if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
> ++ if (s->error_fname[0] != '|' && strncmp(s->error_fname, "syslog", 6) != 0)
> + tmp = ap_server_root_relative(p, s->error_fname);
> + else
> + tmp = s->error_fname;
>
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Riggs <ji...@riggs.me>.
Luca -
Here's the same thing standardizing on strn?cmp(). Not that you couldn't have done it yourself, but since I had it up, maybe this will save you 30 seconds. ;-)
Index: server/core.c
===================================================================
--- server/core.c (revision 1829439)
+++ server/core.c (working copy)
@@ -4867,7 +4867,8 @@
static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
{
if (!s->error_fname || s->error_fname[0] == '|'
- || strcmp(s->error_fname, "syslog") == 0) {
+ || strcmp(s->error_fname, "syslog") == 0
+ || strncmp(s->error_fname, "syslog:", 7) == 0) {
return APR_SUCCESS;
}
else {
@@ -5281,7 +5282,9 @@
apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
tmp = ap_server_root_relative(p, sconf->ap_document_root);
apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
- if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
+ if (s->error_fname[0] != '|'
+ && strcmp(s->error_fname, "syslog") != 0
+ && strncmp(s->error_fname, "syslog:", 7) != 0)
tmp = ap_server_root_relative(p, s->error_fname);
else
tmp = s->error_fname;
@@ -5456,4 +5459,3 @@
core_cmds, /* command apr_table_t */
register_hooks /* register hooks */
};
-
Index: server/log.c
===================================================================
--- server/log.c (revision 1829439)
+++ server/log.c (working copy)
@@ -396,7 +396,8 @@
}
#ifdef HAVE_SYSLOG
- else if (!strncasecmp(s->error_fname, "syslog", 6)) {
+ else if (strcmp(s->error_fname, "syslog") == 0
+ || strncmp(s->error_fname, "syslog:", 7) == 0) {
if ((fname = strchr(s->error_fname, ':'))) {
/* s->error_fname could be [level]:[tag] (see #60525) */
const char *tag;
> On 18 Apr 2018, at 13:32, Luca Toscano <to...@gmail.com> wrote:
>
> Thanks a lot Jim! I like your code change and the extra checks, but I'd prefer to use strncmp if possible, also in log.c.
> Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in CHANGES too, your name should be on it :).
>
> Luca
>
> 2018-04-18 18:36 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> Fair enough. I'm fine standardizing either way. strn?cmp() is probably more "correct". As it stands, though, the check in core is not actually checking everything that log.c will handle.
>
>
>> On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>>
>> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <ji...@riggs.me> wrote:
>>> I didn't think of this before, but there is one edge case this would miss: if someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It appears that this is already broken in server/log.c, though. Also, log.c is using str(n)casecmp. The following would make things consistent and handle this weird edge case. Thoughts?
>>>
>>> Index: server/core.c
>>> ===================================================================
>>> --- server/core.c (revision 1829431)
>>> +++ server/core.c (working copy)
>>> @@ -4867,7 +4867,8 @@
>>> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
>>> {
>>> if (!s->error_fname || s->error_fname[0] == '|'
>>> - || strcmp(s->error_fname, "syslog") == 0) {
>>> + || strcasecmp(s->error_fname, "syslog") == 0
>>> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>>> return APR_SUCCESS;
>>> }
>>> else {
>>> @@ -5281,7 +5282,9 @@
>>> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
>>> tmp = ap_server_root_relative(p, sconf->ap_document_root);
>>> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
>>> - if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
>>> + if (s->error_fname[0] != '|'
>>> + && strcasecmp(s->error_fname, "syslog") != 0
>>> + && strncasecmp(s->error_fname, "syslog:", 7) != 0)
>>> tmp = ap_server_root_relative(p, s->error_fname);
>>> else
>>> tmp = s->error_fname;
>>> @@ -5456,4 +5459,3 @@
>>> core_cmds, /* command apr_table_t */
>>> register_hooks /* register hooks */
>>> };
>>> -
>>> Index: server/log.c
>>> ===================================================================
>>> --- server/log.c (revision 1829431)
>>> +++ server/log.c (working copy)
>>> @@ -396,7 +396,8 @@
>>> }
>>>
>>> #ifdef HAVE_SYSLOG
>>> - else if (!strncasecmp(s->error_fname, "syslog", 6)) {
>>> + else if (strcasecmp(s->error_fname, "syslog") == 0
>>> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>>> if ((fname = strchr(s->error_fname, ':'))) {
>>> /* s->error_fname could be [level]:[tag] (see #60525) */
>>> const char *tag;
>>
>> Shouldn't we normalize the use of strcmp instead of strcasecmp?
>> In any case it must be entirely normalized to one or the other.
>>
>> Linux is a case-sensitive OS in the first place, and if configured
>> with SYSLOG: today it is broken when you hit core, which ignores
>> that code path. Since the behavior has always been syslog: I'm
>> not seeing a benefit to case insensitivity.
>
>
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Riggs <ji...@riggs.me>.
> On 20 Apr 2018, at 09:52, Luca Toscano <to...@gmail.com> wrote:
> 2018-04-20 16:27 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> > On 20 Apr 2018, at 08:53, Jim Jagielski <ji...@jaguNET.com> wrote:
> >
> > Sorry for coming in late, but what is the exact issue we are trying to solve again? My understanding was that if someone wanted something like
> >
> > ErrorLog "syslog-httpd.log"
> >
> > that the current implementation would, incorrectly, send the log data to syslogd. Is that right?
>
> Luca is working PR 62102 which has to do with "syslog:...:...", but that unveiled an inconsistency between core.c and log.c. Before his proposed patch in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is doing strncasecmp(). We have been discussing standardizing both on strn?cmp(), but that would potentially lead to "breaking" configs that use "SYSLOG" rather than "syslog".
>
>
> I don't believe that they two things are separate (core/log.c), since the former checks for ErrorLog's parameter sanity and the latter sets it, so it would be weird in my opinion if the two logic were different. This is why I liked your consistency proposal Jim, and applied to my patch. In our docs we clearly specify to use "syslog" in lowercase, so as far as I can see it using "SYSLOG" would not be something that people should use..
Agreed, and I believe we should go forward with this. I just wanted to at least bring it up, since regressions and breaking existing configs has been a touchy subject lately. :-)
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Luca Toscano <to...@gmail.com>.
2018-04-20 16:27 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> > On 20 Apr 2018, at 08:53, Jim Jagielski <ji...@jaguNET.com> wrote:
> >
> > Sorry for coming in late, but what is the exact issue we are trying to
> solve again? My understanding was that if someone wanted something like
> >
> > ErrorLog "syslog-httpd.log"
> >
> > that the current implementation would, incorrectly, send the log data to
> syslogd. Is that right?
>
> Luca is working PR 62102 which has to do with "syslog:...:...", but that
> unveiled an inconsistency between core.c and log.c. Before his proposed
> patch in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is
> doing strncasecmp(). We have been discussing standardizing both on
> strn?cmp(), but that would potentially lead to "breaking" configs that use
> "SYSLOG" rather than "syslog".
>
>
I don't believe that they two things are separate (core/log.c), since the
former checks for ErrorLog's parameter sanity and the latter sets it, so it
would be weird in my opinion if the two logic were different. This is why I
liked your consistency proposal Jim, and applied to my patch. In our docs
we clearly specify to use "syslog" in lowercase, so as far as I can see it
using "SYSLOG" would not be something that people should use..
Luca
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Riggs <ji...@riggs.me>.
> On 20 Apr 2018, at 08:53, Jim Jagielski <ji...@jaguNET.com> wrote:
>
> Sorry for coming in late, but what is the exact issue we are trying to solve again? My understanding was that if someone wanted something like
>
> ErrorLog "syslog-httpd.log"
>
> that the current implementation would, incorrectly, send the log data to syslogd. Is that right?
Luca is working PR 62102 which has to do with "syslog:...:...", but that unveiled an inconsistency between core.c and log.c. Before his proposed patch in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is doing strncasecmp(). We have been discussing standardizing both on strn?cmp(), but that would potentially lead to "breaking" configs that use "SYSLOG" rather than "syslog".
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Jagielski <ji...@jaguNET.com>.
Sorry for coming in late, but what is the exact issue we are trying to solve again? My understanding was that if someone wanted something like
ErrorLog "syslog-httpd.log"
that the current implementation would, incorrectly, send the log data to syslogd. Is that right?
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Riggs <ji...@riggs.me>.
> On 20 Apr 2018, at 01:42, Luca Toscano <to...@gmail.com> wrote:
> 2018-04-19 17:49 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> Luca -
>
> Here's the same thing standardizing on strn?cmp(). Not that you couldn't have done it yourself, but since I had it up, maybe this will save you 30 seconds. ;-)
>
>
> Thanks a lot! I added your last suggestions to r1829626 and also a CHANGES entry. I tested it on my local environment and it seems working fine, let me know if everything looks good or if anything is missing.
I'm fine with it, *BUT* in light of our recent discussions about backports/fixes causing regressions in existing configs, this could cause another. I agree with what we have done in this patch, but we could potentially break existing configs, no? For example:
ErrorLog "SYSLOG"
This will send to syslog before the patch but will send to file <server_root>/SYSLOG after. Ugh. So, should we not touch server/log.c???
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Luca Toscano <to...@gmail.com>.
2018-04-19 17:49 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> Luca -
>
> Here's the same thing standardizing on strn?cmp(). Not that you couldn't
> have done it yourself, but since I had it up, maybe this will save you 30
> seconds. ;-)
>
>
Thanks a lot! I added your last suggestions to r1829626 and also a CHANGES
entry. I tested it on my local environment and it seems working fine, let
me know if everything looks good or if anything is missing.
Luca
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Riggs <ji...@riggs.me>.
Luca -
Here's the same thing standardizing on strn?cmp(). Not that you couldn't have done it yourself, but since I had it up, maybe this will save you 30 seconds. ;-)
Index: server/core.c
===================================================================
--- server/core.c (revision 1829439)
+++ server/core.c (working copy)
@@ -4867,7 +4867,8 @@
static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
{
if (!s->error_fname || s->error_fname[0] == '|'
- || strcmp(s->error_fname, "syslog") == 0) {
+ || strcmp(s->error_fname, "syslog") == 0
+ || strncmp(s->error_fname, "syslog:", 7) == 0) {
return APR_SUCCESS;
}
else {
@@ -5281,7 +5282,9 @@
apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
tmp = ap_server_root_relative(p, sconf->ap_document_root);
apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
- if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
+ if (s->error_fname[0] != '|'
+ && strcmp(s->error_fname, "syslog") != 0
+ && strncmp(s->error_fname, "syslog:", 7) != 0)
tmp = ap_server_root_relative(p, s->error_fname);
else
tmp = s->error_fname;
@@ -5456,4 +5459,3 @@
core_cmds, /* command apr_table_t */
register_hooks /* register hooks */
};
-
Index: server/log.c
===================================================================
--- server/log.c (revision 1829439)
+++ server/log.c (working copy)
@@ -396,7 +396,8 @@
}
#ifdef HAVE_SYSLOG
- else if (!strncasecmp(s->error_fname, "syslog", 6)) {
+ else if (strcmp(s->error_fname, "syslog") == 0
+ || strncmp(s->error_fname, "syslog:", 7) == 0) {
if ((fname = strchr(s->error_fname, ':'))) {
/* s->error_fname could be [level]:[tag] (see #60525) */
const char *tag;
> On 18 Apr 2018, at 13:32, Luca Toscano <to...@gmail.com> wrote:
>
> Thanks a lot Jim! I like your code change and the extra checks, but I'd prefer to use strncmp if possible, also in log.c.
> Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in CHANGES too, your name should be on it :).
>
> Luca
>
> 2018-04-18 18:36 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> Fair enough. I'm fine standardizing either way. strn?cmp() is probably more "correct". As it stands, though, the check in core is not actually checking everything that log.c will handle.
>
>
> > On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> >
> > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <ji...@riggs.me> wrote:
> >> I didn't think of this before, but there is one edge case this would miss: if someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It appears that this is already broken in server/log.c, though. Also, log.c is using str(n)casecmp. The following would make things consistent and handle this weird edge case. Thoughts?
> >>
> >> Index: server/core.c
> >> ===================================================================
> >> --- server/core.c (revision 1829431)
> >> +++ server/core.c (working copy)
> >> @@ -4867,7 +4867,8 @@
> >> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
> >> {
> >> if (!s->error_fname || s->error_fname[0] == '|'
> >> - || strcmp(s->error_fname, "syslog") == 0) {
> >> + || strcasecmp(s->error_fname, "syslog") == 0
> >> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> >> return APR_SUCCESS;
> >> }
> >> else {
> >> @@ -5281,7 +5282,9 @@
> >> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
> >> tmp = ap_server_root_relative(p, sconf->ap_document_root);
> >> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
> >> - if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
> >> + if (s->error_fname[0] != '|'
> >> + && strcasecmp(s->error_fname, "syslog") != 0
> >> + && strncasecmp(s->error_fname, "syslog:", 7) != 0)
> >> tmp = ap_server_root_relative(p, s->error_fname);
> >> else
> >> tmp = s->error_fname;
> >> @@ -5456,4 +5459,3 @@
> >> core_cmds, /* command apr_table_t */
> >> register_hooks /* register hooks */
> >> };
> >> -
> >> Index: server/log.c
> >> ===================================================================
> >> --- server/log.c (revision 1829431)
> >> +++ server/log.c (working copy)
> >> @@ -396,7 +396,8 @@
> >> }
> >>
> >> #ifdef HAVE_SYSLOG
> >> - else if (!strncasecmp(s->error_fname, "syslog", 6)) {
> >> + else if (strcasecmp(s->error_fname, "syslog") == 0
> >> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> >> if ((fname = strchr(s->error_fname, ':'))) {
> >> /* s->error_fname could be [level]:[tag] (see #60525) */
> >> const char *tag;
> >
> > Shouldn't we normalize the use of strcmp instead of strcasecmp?
> > In any case it must be entirely normalized to one or the other.
> >
> > Linux is a case-sensitive OS in the first place, and if configured
> > with SYSLOG: today it is broken when you hit core, which ignores
> > that code path. Since the behavior has always been syslog: I'm
> > not seeing a benefit to case insensitivity.
>
>
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Luca Toscano <to...@gmail.com>.
Thanks a lot Jim! I like your code change and the extra checks, but I'd
prefer to use strncmp if possible, also in log.c.
Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in
CHANGES too, your name should be on it :).
Luca
2018-04-18 18:36 GMT+02:00 Jim Riggs <ji...@riggs.me>:
> Fair enough. I'm fine standardizing either way. strn?cmp() is probably
> more "correct". As it stands, though, the check in core is not actually
> checking everything that log.c will handle.
>
>
> > On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> >
> > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <ji...@riggs.me> wrote:
> >> I didn't think of this before, but there is one edge case this would
> miss: if someone (for whatever reason) wants a relative ErrorLog *file*
> named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog
> "syslog.log"'. It appears that this is already broken in server/log.c,
> though. Also, log.c is using str(n)casecmp. The following would make things
> consistent and handle this weird edge case. Thoughts?
> >>
> >> Index: server/core.c
> >> ===================================================================
> >> --- server/core.c (revision 1829431)
> >> +++ server/core.c (working copy)
> >> @@ -4867,7 +4867,8 @@
> >> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
> >> {
> >> if (!s->error_fname || s->error_fname[0] == '|'
> >> - || strcmp(s->error_fname, "syslog") == 0) {
> >> + || strcasecmp(s->error_fname, "syslog") == 0
> >> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> >> return APR_SUCCESS;
> >> }
> >> else {
> >> @@ -5281,7 +5282,9 @@
> >> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
> >> tmp = ap_server_root_relative(p, sconf->ap_document_root);
> >> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
> >> - if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog")
> != 0)
> >> + if (s->error_fname[0] != '|'
> >> + && strcasecmp(s->error_fname, "syslog") != 0
> >> + && strncasecmp(s->error_fname, "syslog:", 7) != 0)
> >> tmp = ap_server_root_relative(p, s->error_fname);
> >> else
> >> tmp = s->error_fname;
> >> @@ -5456,4 +5459,3 @@
> >> core_cmds, /* command apr_table_t */
> >> register_hooks /* register hooks */
> >> };
> >> -
> >> Index: server/log.c
> >> ===================================================================
> >> --- server/log.c (revision 1829431)
> >> +++ server/log.c (working copy)
> >> @@ -396,7 +396,8 @@
> >> }
> >>
> >> #ifdef HAVE_SYSLOG
> >> - else if (!strncasecmp(s->error_fname, "syslog", 6)) {
> >> + else if (strcasecmp(s->error_fname, "syslog") == 0
> >> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> >> if ((fname = strchr(s->error_fname, ':'))) {
> >> /* s->error_fname could be [level]:[tag] (see #60525) */
> >> const char *tag;
> >
> > Shouldn't we normalize the use of strcmp instead of strcasecmp?
> > In any case it must be entirely normalized to one or the other.
> >
> > Linux is a case-sensitive OS in the first place, and if configured
> > with SYSLOG: today it is broken when you hit core, which ignores
> > that code path. Since the behavior has always been syslog: I'm
> > not seeing a benefit to case insensitivity.
>
>
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Riggs <ji...@riggs.me>.
Fair enough. I'm fine standardizing either way. strn?cmp() is probably more "correct". As it stands, though, the check in core is not actually checking everything that log.c will handle.
> On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <ji...@riggs.me> wrote:
>> I didn't think of this before, but there is one edge case this would miss: if someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It appears that this is already broken in server/log.c, though. Also, log.c is using str(n)casecmp. The following would make things consistent and handle this weird edge case. Thoughts?
>>
>> Index: server/core.c
>> ===================================================================
>> --- server/core.c (revision 1829431)
>> +++ server/core.c (working copy)
>> @@ -4867,7 +4867,8 @@
>> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
>> {
>> if (!s->error_fname || s->error_fname[0] == '|'
>> - || strcmp(s->error_fname, "syslog") == 0) {
>> + || strcasecmp(s->error_fname, "syslog") == 0
>> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>> return APR_SUCCESS;
>> }
>> else {
>> @@ -5281,7 +5282,9 @@
>> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
>> tmp = ap_server_root_relative(p, sconf->ap_document_root);
>> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
>> - if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
>> + if (s->error_fname[0] != '|'
>> + && strcasecmp(s->error_fname, "syslog") != 0
>> + && strncasecmp(s->error_fname, "syslog:", 7) != 0)
>> tmp = ap_server_root_relative(p, s->error_fname);
>> else
>> tmp = s->error_fname;
>> @@ -5456,4 +5459,3 @@
>> core_cmds, /* command apr_table_t */
>> register_hooks /* register hooks */
>> };
>> -
>> Index: server/log.c
>> ===================================================================
>> --- server/log.c (revision 1829431)
>> +++ server/log.c (working copy)
>> @@ -396,7 +396,8 @@
>> }
>>
>> #ifdef HAVE_SYSLOG
>> - else if (!strncasecmp(s->error_fname, "syslog", 6)) {
>> + else if (strcasecmp(s->error_fname, "syslog") == 0
>> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>> if ((fname = strchr(s->error_fname, ':'))) {
>> /* s->error_fname could be [level]:[tag] (see #60525) */
>> const char *tag;
>
> Shouldn't we normalize the use of strcmp instead of strcasecmp?
> In any case it must be entirely normalized to one or the other.
>
> Linux is a case-sensitive OS in the first place, and if configured
> with SYSLOG: today it is broken when you hit core, which ignores
> that code path. Since the behavior has always been syslog: I'm
> not seeing a benefit to case insensitivity.
Re: svn commit: r1829430 -
/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Apr 18, 2018, at 11:15 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
>
> Shouldn't we normalize the use of strcmp instead of strcasecmp?
> In any case it must be entirely normalized to one or the other.
>
> Linux is a case-sensitive OS in the first place, and if configured
> with SYSLOG: today it is broken when you hit core, which ignores
> that code path. Since the behavior has always been syslog: I'm
> not seeing a benefit to case insensitivity.
macOS is typically case aware but not necessarily case sensitive.
For example, both
/tmp/foobarski
and
/tmp/FooBARskI
are seen as the same file, but case is maintained.
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <ji...@riggs.me> wrote:
> I didn't think of this before, but there is one edge case this would miss: if someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It appears that this is already broken in server/log.c, though. Also, log.c is using str(n)casecmp. The following would make things consistent and handle this weird edge case. Thoughts?
>
> Index: server/core.c
> ===================================================================
> --- server/core.c (revision 1829431)
> +++ server/core.c (working copy)
> @@ -4867,7 +4867,8 @@
> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
> {
> if (!s->error_fname || s->error_fname[0] == '|'
> - || strcmp(s->error_fname, "syslog") == 0) {
> + || strcasecmp(s->error_fname, "syslog") == 0
> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> return APR_SUCCESS;
> }
> else {
> @@ -5281,7 +5282,9 @@
> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
> tmp = ap_server_root_relative(p, sconf->ap_document_root);
> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
> - if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
> + if (s->error_fname[0] != '|'
> + && strcasecmp(s->error_fname, "syslog") != 0
> + && strncasecmp(s->error_fname, "syslog:", 7) != 0)
> tmp = ap_server_root_relative(p, s->error_fname);
> else
> tmp = s->error_fname;
> @@ -5456,4 +5459,3 @@
> core_cmds, /* command apr_table_t */
> register_hooks /* register hooks */
> };
> -
> Index: server/log.c
> ===================================================================
> --- server/log.c (revision 1829431)
> +++ server/log.c (working copy)
> @@ -396,7 +396,8 @@
> }
>
> #ifdef HAVE_SYSLOG
> - else if (!strncasecmp(s->error_fname, "syslog", 6)) {
> + else if (strcasecmp(s->error_fname, "syslog") == 0
> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> if ((fname = strchr(s->error_fname, ':'))) {
> /* s->error_fname could be [level]:[tag] (see #60525) */
> const char *tag;
Shouldn't we normalize the use of strcmp instead of strcasecmp?
In any case it must be entirely normalized to one or the other.
Linux is a case-sensitive OS in the first place, and if configured
with SYSLOG: today it is broken when you hit core, which ignores
that code path. Since the behavior has always been syslog: I'm
not seeing a benefit to case insensitivity.