Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <3FD7EF5E.4060203@modperlcookbook.org>
Date: Wed, 10 Dec 2003 23:15:26 -0500
From: Geoffrey Young <geoff@modperlcookbook.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20030225
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: [PATCH] catching malformed container directives
References: <3FD4A376.1050408@modperlcookbook.org>
 <3FD4A00E.5060607@modperlcookbook.org> <20031208171119.000021e1.nd@perlig.de>
 <3FD4A376.1050408@modperlcookbook.org>
 <5.2.0.9.2.20031209105803.027e2eb8@pop3.rowe-clan.net>
 <3FD65165.80901@modperlcookbook.org>
In-Reply-To: <3FD65165.80901@modperlcookbook.org>
Content-Type: multipart/mixed;
 boundary="------------040902070205080907080803"

This is a multi-part message in MIME format.
--------------040902070205080907080803
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit


>>Now you have me thinking.  For Apache 2.1 (perhaps 2.0) I'd like to see that
>>particular nonsense go away.  I sympathize with Andr�'s observation that it's
>>useful, but what he wants to do can be accomplished with
>>
>><IfDefine NEVER>
>>    DangerousDirective
>></IfDefine>
>>
>>which serves the same purpose, but it much more legible.
> 
> 
> not to speak for andre, but he pointed out to me on irc that what he was
> after was an <IfDefine> that could not be overridden with -D, and I suppose
> -DNEVER would expose the config block.  or are you suggesting a literal
> "<IfDefine NEVER>" as a special case?  one thing I suggested was perhaps
> using <IfDefine 0>, but he pointed out that -D0 works (but -D"" doesn't).
> so maybe we can make -D0 not work as well and keep with something that feels
> programmatically familiar.

yet another try :)

this patch makes 'httpd -D0' invalid, thus making <IfDefine 0> a special
define case that is guaranteed to evaluate to false.  the rest remains as
before - arguments are enforced across all containers.

it actually feels a bit strange to fail the command line args without some
kind of message, so I suppose it might be wiser to implement this in core.c
instead, tossing an error message to the error_log if "0" is both caught and
defined.  but for the moment I guess I'm just seeing if the idea is
appealing, after which the implementation can be adjusted as required.

--Geoff

--------------040902070205080907080803
Content-Type: text/plain;
 name="missing_args3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="missing_args3.patch"

Index: server/core.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/core.c,v
retrieving revision 1.253
diff -u -r1.253 core.c
--- server/core.c	10 Dec 2003 22:40:33 -0000	1.253
+++ server/core.c	11 Dec 2003 04:06:57 -0000
@@ -1561,6 +1561,15 @@
                        "> directive missing closing '>'", NULL);
 }
 
+/*
+ * Report a missing args in '<Foo >' syntax error.
+ */
+static char *missing_container_arg(cmd_parms *cmd)
+{
+    return apr_pstrcat(cmd->pool, cmd->cmd->name,
+                       "> directive requires additional arguments", NULL);
+}
+
 AP_CORE_DECLARE_NONSTD(const char *) ap_limit_section(cmd_parms *cmd,
                                                       void *dummy,
                                                       const char *arg)
@@ -1582,6 +1591,10 @@
 
     limited_methods = apr_pstrndup(cmd->pool, arg, endp - arg);
 
+    if (!limited_methods[0]) {
+        return missing_container_arg(cmd);
+    }
+
     while (limited_methods[0]) {
         char *method = ap_getword_conf(cmd->pool, &limited_methods);
         int methnum;
@@ -1649,6 +1662,10 @@
 
     arg = apr_pstrndup(cmd->pool, arg, endp - arg);
 
+    if (!arg[0]) {
+        return missing_container_arg(cmd);
+    }
+
     if (!arg) {
         if (thiscmd->cmd_data)
             return "<DirectoryMatch > block must specify a path";
@@ -1743,6 +1760,10 @@
 
     arg = apr_pstrndup(cmd->pool, arg, endp - arg);
 
+    if (!arg[0]) {
+        return missing_container_arg(cmd);
+    }
+
     cmd->path = ap_getword_conf(cmd->pool, &arg);
     cmd->override = OR_ALL|ACCESS_CONF;
 
@@ -1802,6 +1823,10 @@
 
     arg = apr_pstrndup(cmd->pool, arg, endp - arg);
 
+    if (!arg[0]) {
+        return missing_container_arg(cmd);
+    }
+
     cmd->path = ap_getword_conf(cmd->pool, &arg);
     /* Only if not an .htaccess file */
     if (!old_path) {
@@ -1867,6 +1892,10 @@
         arg++;
     }
 
+    if (!arg[0]) {
+        return missing_container_arg(cmd);
+    }
+
     found = ap_find_linked_module(arg);
 
     if ((!not && found) || (not && !found)) {
@@ -1918,6 +1947,10 @@
         arg++;
     }
 
+    if (!arg[0]) {
+        return missing_container_arg(cmd);
+    }
+
     defined = ap_exists_config_define(arg);
     if ((!not && defined) || (not && !defined)) {
         ap_directive_t *parent = NULL;
@@ -1955,6 +1988,10 @@
     }
 
     arg = apr_pstrndup(cmd->pool, arg, endp - arg);
+
+    if (!arg[0]) {
+        return missing_container_arg(cmd);
+    }
 
     /* FIXME: There's another feature waiting to happen here -- since you
         can now put multiple addresses/names on a single <VirtualHost>
Index: server/main.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/main.c,v
retrieving revision 1.148
diff -u -r1.148 main.c
--- server/main.c	4 Dec 2003 03:05:42 -0000	1.148
+++ server/main.c	11 Dec 2003 04:06:57 -0000
@@ -505,11 +505,17 @@
             break;
 
         case 'D':
-            new = (char **)apr_array_push(ap_server_config_defines);
-            *new = apr_pstrdup(pcommands, optarg);
-            /* Setting -D DUMP_VHOSTS is equivalent to setting -S */
-            if (strcmp(optarg, "DUMP_VHOSTS") == 0)
-                configtestonly = 1;
+            /* disallow -D0 so that <IfDefine 0> cannot be overridden */
+            if (strcmp(optarg, "0")) {
+                new = (char **)apr_array_push(ap_server_config_defines);
+                *new = apr_pstrdup(pcommands, optarg);
+                /* Setting -D DUMP_VHOSTS is equivalent to setting -S */
+                if (strcmp(optarg, "DUMP_VHOSTS") == 0)
+                    configtestonly = 1;
+            }
+            else {
+                usage(process);
+            }
             break;
 
         case 'e':

--------------040902070205080907080803--