Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

2018-04-20 Thread Jim Riggs
> On 20 Apr 2018, at 09:52, Luca Toscano  wrote:
> 2018-04-20 16:27 GMT+02:00 Jim Riggs :
> > On 20 Apr 2018, at 08:53, Jim Jagielski  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

2018-04-20 Thread Luca Toscano
2018-04-20 16:27 GMT+02:00 Jim Riggs :

> > On 20 Apr 2018, at 08:53, Jim Jagielski  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

2018-04-20 Thread Jim Riggs
> On 20 Apr 2018, at 08:53, Jim Jagielski  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

2018-04-20 Thread Jim Jagielski
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

2018-04-20 Thread Jim Jagielski


> On Apr 18, 2018, at 11:15 AM, William A Rowe Jr  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

2018-04-20 Thread Jim Riggs
> On 20 Apr 2018, at 01:42, Luca Toscano  wrote:
> 2018-04-19 17:49 GMT+02:00 Jim Riggs :
> 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 
/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

2018-04-20 Thread Luca Toscano
2018-04-19 17:49 GMT+02:00 Jim Riggs :

> 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

2018-04-19 Thread Jim Riggs
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  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 :
> 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  wrote:
>> 
>> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs  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;

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

2018-04-19 Thread Jim Riggs
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  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 :
> 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  wrote:
> > 
> > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs  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, ':'))) {
> >> 

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

2018-04-18 Thread Luca Toscano
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 :

> 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  wrote:
> >
> > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs  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

2018-04-18 Thread Jim Riggs
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  wrote:
> 
> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs  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

2018-04-18 Thread William A Rowe Jr
On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs  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

2018-04-18 Thread Jim Riggs
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, elu...@apache.org wrote:
> 
> Author: elukey
> Date: Wed Apr 18 09:57:08 2018
> New Revision: 1829430
> 
> URL: http://svn.apache.org/viewvc?rev=1829430=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=1829429=1829430=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;
>