perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Torsten Foertsch <torsten.foert...@gmx.net>
Subject explaining the dcfg bug (was: What can I do ...)
Date Wed, 09 May 2007 10:30:52 GMT
Hi Fred,

On Wednesday 09 May 2007 10:42, Fred Moyer wrote:
> I have been following this somewhat, but not grokking all of it.
>
> Can you write a test that shows the change and its effect?
>
> I was poking around that area of the code for the stacked handlers bug
> so I understand some of it but a test would be really be helpful here.

Well, I have thought of that myself but I was not able to produce a simple 
test case. It is simple to get the memory corrupted (just call 
$r->push_handlers twice for the same phase) but it's hard to predict when the 
segfault will occur since it depends completely on your memory mgmt 
implementation.

But I can explain the bug more thoroughly.

modperl_handler_lookup_handlers is declared this way:

MpAV **modperl_handler_lookup_handlers(modperl_config_dir_t *dcfg,
                                       modperl_config_srv_t *scfg,
                                       modperl_config_req_t *rcfg,
                                       apr_pool_t *p,
                                       int type, int idx,
                                       modperl_handler_action_e action,
                                       const char **desc)

It is called at startup to configure handlers in server/per_dir config and at 
request time to configure handlers in request config.

At startup when there is no request it is called with rcfg==0. p is in this 
case a longer living pool.

At request time it is passed rcfg!=0 and the request pool in p.

The function initializes avp=0 and ravp=0.

Then it says

        avp = &dcfg->handlers_per_dir[idx];
        if (rcfg) {
            ravp = &rcfg->handlers_per_dir[idx];
        }

(The scfg->handlers_per_srv case is similar)

So, avp now points to an array element inside dcfg while ravp at request time 
points to an array element inside rcfg. At startup it remains 0.

Later the function ensures that avp!=0 and then does this

        if (ravp && !*ravp) {
            if (*avp) {
                /* merge with existing configured handlers */
                *ravp = apr_array_copy(p, *avp);
            }
            else {
                /* no request handlers have been previously pushed or set */
                *ravp = modperl_handler_array_new(p);
            }
        }
        else if (!*avp) {
            /* directly modify the configuration at startup time */
            *avp = modperl_handler_array_new(p);
        }

The arrays dcfg->handlers_per_dir and rcfg->handlers_per_dir are initially set 
to all NULL pointers. If an element is 0 it means there is no handler set for 
this phase. Otherwise it points to an apr_array.

So, at startup time we have to make sure that *avp points to an apr_array and 
return that while at request time *ravp should point to an apr_array and that 
should be returned.

Let's first look what happens at startup. rcfg==NULL and hence ravp==NULL. 
The "else if" checks whether the appropriate element of dcfg is already 
initialized. If not it is assigned an empty apr_array. Here the program works 
correct.

Now at request time. rcfg!=NULL and hence ravp!=NULL. *ravp may be NULL if 
there is no handler set yet.

The "if (ravp && !*ravp)" works correct if *ravp is NULL. In this case (the 
first time $r->push_handlers is called) *ravp gets initialized.

But if the code is run a second time the if condition is false because *ravp 
is already set. In this case we must do nothing but simply return ravp.

So we hit the else if branch. If now *avp is NULL we initialize it. And this 
is wrong. To cite from Nick Kew book:

  When processing a request, use the fields of the request_rec - in
  particular, the request pool and the request configuration vector.
  Treat everything else as read-only.

In our case we modify a structure which is valid until server shutdown and 
write there a pointer that is allocated from the request pool since p in

            *avp = modperl_handler_array_new(p);

is the request pool.

So how can it be that *avp is NULL at request time? That simply means there 
has been no handler configured at startup time.

What possible solutions are there? Let's have a look at the code for the other

      case MP_HANDLER_ACTION_SET:

Here it says

        if (ravp) {
        }
        else if (*avp) {
        }
        else {
        }

This is correct because no else branch is entered if ravp!=NULL.

For the ACTION_PUSH case we can do it the same way:

         if (ravp) {
            if( !*ravp ) {
                if (*avp) {
                    /* merge with existing configured handlers */
                    *ravp = apr_array_copy(p, *avp);
                }
                else {
                    /* no request handlers have been previously pushed or set 
*/
                    *ravp = modperl_handler_array_new(p);
                }
            }
        }
        else if (!*avp) {
            /* directly modify the configuration at startup time */
            *avp = modperl_handler_array_new(p);
        }

or the way the original patch did:

        if (ravp && !*ravp) {
        }
        else if (ravp) {
	    /* ravp is already initialized: do nothing */
        }
        else if (!*avp) {
        }

So the bug reduces to 

if (a) {
  if (b) {
  }
} else if (c) {
}

not being the same as 

if (a && b) {
} else if (c) {
}

Hope that helps,
Torsten

Mime
View raw message