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 <j...@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 <jim...@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.
>
>
