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
pgp74MgvTfTGH.pgp
Description: PGP signature
