Project

General

Profile

Bug #2598

Semantics of else clause looks strange

Added by Gwenlliana over 2 years ago. Updated over 1 year ago.

Status:
Fixed
Priority:
High
Assignee:
-
Category:
-
Target version:
Start date:
2014-11-01
Due date:
% Done:

100%

Estimated time:
Missing in 1.5.x:

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.

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

Associated revisions

Revision 3081 (diff)
Added by stbuehler over 1 year ago

[core] never evaluate else branches until the previous branches are aready (fixes #2598)

The first condition which evaluates true in any if-else... condition
chain short-circuits the chain, and any remaining conditions in the
chain are marked false.

Previous conditions in if-else condition chaining must be evaluatable
(to true or false) -- must not remain in unset (not yet evaluatable)
state -- prior to evaluating later conditions. Since any true
condition short-circuits remaining conditions, all prev conditions
must be false prior to evaluating later conditions.

From: Glenn Strauss <>

Revision 1c01a42a (diff)
Added by gstrauss over 1 year ago

[core] never evaluate else branches until the previous branches are aready (fixes #2598)

The first condition which evaluates true in any if-else... condition
chain short-circuits the chain, and any remaining conditions in the
chain are marked false.

Previous conditions in if-else condition chaining must be evaluatable
(to true or false) -- must not remain in unset (not yet evaluatable)
state -- prior to evaluating later conditions. Since any true
condition short-circuits remaining conditions, all prev conditions
must be false prior to evaluating later conditions.

From: Glenn Strauss <>

git-svn-id: svn://svn.lighttpd.net/lighttpd/branches/lighttpd-1.4.x@3081 152afb58-edef-0310-8abb-c4023f1b3aa9

Revision 3083 (diff)
Added by stbuehler over 1 year ago

[core] improve conditional enabling (thx Gwenlliana, #2598)

instead of looping over all config blocks for each conditional var that
gets enabled, enable them all and run over them once.

Right now it seems we actually set all variables at once in normal
config handling (SNI only sets a subset); future modifications
might introduce new variables which are activated at a later stage
(physical path related for example).

From: Stefan Bühler <>

Revision c033a196 (diff)
Added by stbuehler over 1 year ago

[core] improve conditional enabling (thx Gwenlliana, #2598)

instead of looping over all config blocks for each conditional var that
gets enabled, enable them all and run over them once.

Right now it seems we actually set all variables at once in normal
config handling (SNI only sets a subset); future modifications
might introduce new variables which are activated at a later stage
(physical path related for example).

From: Stefan Bühler <>

git-svn-id: svn://svn.lighttpd.net/lighttpd/branches/lighttpd-1.4.x@3083 152afb58-edef-0310-8abb-c4023f1b3aa9

History

#1 Updated by stbuehler over 2 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.

#2 Updated by Gwenlliana over 2 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;
        }

#3 Updated by Gwenlliana over 2 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 

#4 Updated by gstrauss over 1 year 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.

#5 Updated by gstrauss over 1 year 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) {

#6 Updated by stbuehler over 1 year 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.

#7 Updated by gstrauss over 1 year 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.

#8 Updated by stbuehler over 1 year 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 :(

#9 Updated by stbuehler over 1 year ago

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

Applied in changeset r3081.

Also available in: Atom