Project

General

Profile

Actions
Copy link

Bug #2598

closed

Semantics of else clause looks strange

Added by Gwenlliana over 9 years ago. Updated about 8 years ago.

Status:
Fixed
Priority:
High
Category:
-
Target version:
1.4.40
ASK QUESTIONS IN Forums:

Description

According to lighttpd configuration documentation, we can use else to group multiple conditional expressions together as a larger expression, the semantics of cond_a else cond_b should be similar with if cond_a else if cond_b in most programming languages. However in some circumstances, cond_a and cond_b can both take effect.

Here is a fragment taken from a lighttpd configuration file to illustrate this issue. We have a conditional clause containing two branches: the first one matches urls beginning with “/repo/“, this is the directory where we want to dir-listing; the last one matches all requests with “http” scheme, in this branch we disabled symbolic link following.

server.document-root = “/path/to/docroot“
server.port = 3000

$HTTP["url"] =~ "^/repo/" {
    dir-listing.activate = "enable" 
}
else $HTTP["scheme"] == "http" {
    server.follow-symlink = "disable" 
}

When we access /repo/, the first case should be matched and we should enable dir-listing while discarding the second branch. If there is a symbolic link in repo/, we should be able to follow it without any problem (follow-symlink is enabled by default), however I got a 403 Forbidden instead, which means that both branch took effect. Similar problem would occur when you have a huge tangle of nested blocks containing else conds, which would lead to hard to debug server misconfigurations.

This problem might be caused by multiple passes of connection configuration patching (config_patch_connection in responses.c), running early passes when only partial comp condition was validated might exhibit this “out of order execution” behaviour, so we can solve this problem by running the connection patching pass only once after all comp conditions become valid. an incomplete fix (the SSL part is hard to fix correctly) for this issue was attached and hope it would help.

Files

Download all files
elsecond.patch (4.56 KB) elsecond.patch Gwenlliana, 2014-11-01 14:53
cond_log.txt (5.34 KB) cond_log.txt Gwenlliana, 2014-11-01 15:31
Actions
Copy link
 #1

Updated by stbuehler over 9 years ago

The else branch doesn't (shouldn't) get activated if the first branch wasn't decided yet.

Maybe you can track down the problem with condition (cache) debugging, see DebugVariables.

Actions
Copy link
 #2

Updated by Gwenlliana over 9 years ago

stbuehler wrote:

The else branch doesn't (shouldn't) get activated if the first branch wasn't decided yet.

Maybe you can track down the problem with condition (cache) debugging, see DebugVariables.

Great hint, we should leave else branches unevaluated when previous conditions are not evaluated yet.

I reproduced this issue with debug.log-condition-handling = "enable", debug.log-condition-cache-handling seems to be an 1.5.x only option so I cannot enable it. The log captured was uploaded as attachment.

Maybe a correct fix is to patch config_check_cond_nocache. I see that when parent block is unevaluated, it would leave child expressions unset. maybe we should do the same thing to else branches. A possible fix might be something like this:

        /* make sure prev is checked first */
        config_check_cond_cached(srv, con, dc->prev);

        /* one of prev set me to FALSE */
        switch (con->cond_cache[dc->context_ndx].result) {
        case COND_RESULT_FALSE:
            return con->cond_cache[dc->context_ndx].result;
+        case COND_RESULT_UNSET:
+            return COND_RESULT_UNSET;
        default:
            break;
        }
Actions
Copy link
 #3

Updated by Gwenlliana over 9 years ago

[Wrong patch posted in previous comment, already fixed in place.]

I tried to track down the problem and found that the second branch was activated when the first branch is unset, after reviewing the source code (configfile-glue.c, 263) I found that the else branch would be evaluated when previous branches are not false, which means that if the previous branch is unset, the else branch can still be evaluated. Seems like the unset case was dropped when handling else-cond chains.

Fragment of cond handling logs, HTTP["scheme"] was evaluated when ^/repo/ was still unset:

2014-11-01 22:22:39: (configfile-glue.c.579) === start of condition block === 
2014-11-01 22:22:39: (configfile-glue.c.256) go prev global/HTTPurl=~^/repo/ 
2014-11-01 22:22:39: (configfile-glue.c.273) 2 global/HTTPurl=~^/repo/ nej 
2014-11-01 22:22:39: (configfile-glue.c.530) 1 (uncached) result: unknown 
2014-11-01 22:22:39: (configfile-glue.c.273) 10 global/HTTPscheme==http nej 
2014-11-01 22:22:39: (configfile-glue.c.530) 2 (uncached) result: unknown 
2014-11-01 22:22:39: (configfile-glue.c.579) === start of condition block === 
2014-11-01 22:22:39: (configfile-glue.c.273) 2 global/HTTPurl=~^/repo/ nej 
2014-11-01 22:22:39: (configfile-glue.c.530) 1 (uncached) result: unknown 
2014-11-01 22:22:39: (configfile-glue.c.579) === start of condition block === 
2014-11-01 22:22:39: (configfile-glue.c.256) go prev global/HTTPurl=~^/repo/ 
2014-11-01 22:22:39: (configfile-glue.c.273) 2 global/HTTPurl=~^/repo/ nej 
2014-11-01 22:22:39: (configfile-glue.c.530) 1 (uncached) result: unknown 
2014-11-01 22:22:39: (configfile-glue.c.467) HTTP["scheme"] ( http ) compare to  http 
2014-11-01 22:22:39: (configfile-glue.c.530) 2 (uncached) result: true 
2014-11-01 22:22:39: (configfile-glue.c.579) === start of condition block === 
2014-11-01 22:22:39: (configfile-glue.c.273) 2 global/HTTPurl=~^/repo/ nej 
2014-11-01 22:22:39: (configfile-glue.c.530) 1 (uncached) result: unknown 
2014-11-01 22:22:39: (configfile-glue.c.579) === start of condition block === 
2014-11-01 22:22:39: (configfile-glue.c.537) 2 (cached) result: true

Here is the log fragment when we don't evaluate else conds when previous cond is unset, it won't try to evaluate HTTP["scheme"] branch when the first ^/repo/ branch is left unset.

2014-11-02 17:21:48: (configfile-glue.c.581) === start of condition block === 
2014-11-02 17:21:48: (configfile-glue.c.256) go prev global/HTTPurl=~^/repo/ 
2014-11-02 17:21:48: (configfile-glue.c.275) 2 global/HTTPurl=~^/repo/ nej 
2014-11-02 17:21:48: (configfile-glue.c.532) 1 (uncached) result: unknown 
2014-11-02 17:21:48: (configfile-glue.c.532) 2 (uncached) result: unknown 
2014-11-02 17:21:48: (configfile-glue.c.581) === start of condition block === 
2014-11-02 17:21:48: (configfile-glue.c.275) 2 global/HTTPurl=~^/repo/ nej 
2014-11-02 17:21:48: (configfile-glue.c.532) 1 (uncached) result: unknown 
2014-11-02 17:21:48: (configfile-glue.c.581) === start of condition block === 
2014-11-02 17:21:48: (configfile-glue.c.256) go prev global/HTTPurl=~^/repo/ 
2014-11-02 17:21:48: (configfile-glue.c.275) 2 global/HTTPurl=~^/repo/ nej 
2014-11-02 17:21:48: (configfile-glue.c.532) 1 (uncached) result: unknown 
2014-11-02 17:21:48: (configfile-glue.c.532) 2 (uncached) result: unknown 
2014-11-02 17:21:48: (configfile-glue.c.581) === start of condition block === 
2014-11-02 17:21:48: (configfile-glue.c.275) 2 global/HTTPurl=~^/repo/ nej 
2014-11-02 17:21:48: (configfile-glue.c.532) 1 (uncached) result: unknown

Actions
Copy link
 #4

Updated by gstrauss about 8 years ago

I can understand hesitation to wade into the less-than-friendly condition caching code, but Gwenlliana did a fair amount of footwork here, and I believe the issue is valid. The patch by Gwenlliana looks good to me (visual review only) and will improve the efficiency of runtime config condition evaluation.

I think an additional patch is needed to address the root issue, as any 'else' conditions should not be evaluated unless all prior conditions in the if-else chain have been evaluated to be false. An unset result for a prior condition in the if-else chain should result in the return of an unset result for the subsequent conditions.

--- a/src/configfile-glue.c
+++ b/src/configfile-glue.c
@@ -264,15 +264,8 @@ static cond_result_t config_check_cond_nocache(server *srv, connection *con, dat
                }

                /* make sure prev is checked first */
-               config_check_cond_cached(srv, con, dc->prev);
-
-               /* one of prev set me to FALSE */
-               switch (con->cond_cache[dc->context_ndx].result) {
-               case COND_RESULT_FALSE:
-                       return con->cond_cache[dc->context_ndx].result;
-               default:
-                       break;
-               }
+               if (config_check_cond_cached(srv, con, dc->prev) != COND_RESULT_FALSE) /* prev true or prev unset */
+                       return con->cond_cache[dc->context_ndx].result; /* false if prev true or unset if prev unset */
        }

        if (!con->conditional_is_valid[dc->comp]) {

Separate issue, and not addressed here: mod_extforward use of config_cond_cache_reset_item() does not appear to unset condition cache results for nested conditions or for chained conditions.

Actions
Copy link
 #5

Updated by gstrauss about 8 years ago

Updated patch. It is incorrect to re-evaluate if "one of prev set me to FALSE", so keep that code.

--- a/src/configfile-glue.c
+++ b/src/configfile-glue.c
@@ -264,7 +264,8 @@ static cond_result_t config_check_cond_nocache(server *srv, connection *con, dat
                }

                /* make sure prev is checked first */
-               config_check_cond_cached(srv, con, dc->prev);
+               if (config_check_cond_cached(srv, con, dc->prev) != COND_RESULT_FALSE) /* prev true or prev unset */
+                       return con->cond_cache[dc->context_ndx].result; /* false if prev true or unset if prev unset */

                /* one of prev set me to FALSE */
                switch (con->cond_cache[dc->context_ndx].result) {
Actions
Copy link
 #6

Updated by stbuehler about 8 years ago

  • Priority changed from Normal to High
  • Target version changed from 1.4.x to 1.4.40

I probably should look into this soon... but I don't want to rush it.

Actions
Copy link
 #7

Updated by gstrauss about 8 years ago

Submitted pull request https://github.com/lighttpd/lighttpd1.4/pull/22 with patch to fix root cause.

Please consider Gwenlliana's proposed patch separately, as it still has value in improving performance of runtime condition merging.

(And repeating a note I made above)
Separate issue, and not addressed here: mod_extforward use of config_cond_cache_reset_item() does not appear to unset condition cache results for nested conditions or for chained conditions.

Actions
Copy link
 #8

Updated by stbuehler about 8 years ago

gstrauss: yes, your patch is one way to fix the root cause; the other way would be to reset all variables to the base (index 0) config values in config_patch_connection before looping over the blocks as all the modules do (because in the second example above the second block actually gets "disabled" after the first one gets "enabled" - but the original value from the global block gets never restored).

And yes, the cache reset is broken too :(

Actions
Copy link
 #9

Updated by stbuehler about 8 years ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

Applied in changeset r3081.

Actions
Copy link

Also available in: Atom