segfault analysis

Hi,

I am in the middle of the analysis of a segfault. The
segfault happens in a 
$r->push_handlers(PerlFixupHandler=>sub{}).
Unfortunately it happens not for 
every request but after 13. Finally, I think I have tracked
it down to 
something to talk about.

The modperl_handler_lookup_handlers function looks this:

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)
{
    MpAV **avp = NULL, **ravp = NULL;

[...]

      case MP_HANDLER_ACTION_PUSH:
        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);
        }
        break;

It is called during said push_handlers. For the very first
request it takes 
the "else if (!*avp)" route. This is I think wrong
because it modifies 
dcfg->handlers_per_dir[idx] instead of
dcfg->handlers_per_dir[idx].

p in this case is a request pool. So after the request is
over the pool is 
destroyed but dcfg still holds a pointer allocated from it
in dcfg.

12 requests later a completely different apr_palloc returns
the same pointer 
and in my setup **avp is overwritten by a string.

So, how can it be that the "else if (!*avp)" route
is taken at request time 
and how can it be avoided?

Thanks,
Torsten

Torsten Foertsch wrote:
> On Sunday 06 May 2007 19:11, Torsten Foertsch wrote:
>> [...]
> 
> That happens if there is no handler configured at
startup time but more than 
> one for the same phase at request time.

         if (ravp && !*ravp) {
+	    /* initialize ravp either from avp or as an empty
array */
             if (*avp) {
                 /* merge with existing configured handlers
*/
                 *ravp = apr_array_copy(p, *avp);
 -437,6
+438,9 
                 *ravp = modperl_handler_array_new(p);
             }
         }
+        else if (ravp /* && *ravp */) {
+	    /* ravp is already initialized: do nothing */
+        }
         else if (!*avp) {


Wouldn't something like:

if (ravp && !*ravp) {

}

if (avp && !*avp) {

}

Be more concise ?

------------------------------------------------------------
------------
Philippe M. Chiasson     GPG: F9BFE0C2480E7680
1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.or
g/       m/gozer(apache|cpan|ectoplasm).org/

On Monday 07 May 2007 19:19, Philippe M. Chiasson wrote:
> Torsten Foertsch wrote:
> > On Sunday 06 May 2007 19:11, Torsten Foertsch
wrote:
> >> [...]
> >
> > That happens if there is no handler configured at
startup time but more
> > than one for the same phase at request time.
>
>          if (ravp && !*ravp) {
> +	    /* initialize ravp either from avp or as an empty
array */
>              if (*avp) {
>                  /* merge with existing configured
handlers */
>                  *ravp = apr_array_copy(p, *avp);
>  -437,6 +438,9 
>                  *ravp = modperl_handler_array_new(p);
>              }
>          }
> +        else if (ravp /* && *ravp */) {
> +	    /* ravp is already initialized: do nothing */
> +        }
>          else if (!*avp) {
>
>
> Wouldn't something like:
>
> if (ravp && !*ravp) {
>
> }
>
> if (avp && !*avp) {
>
> }

I wanted to make it more understandable for the next one who
reads the source.

ravp!=0 says: there is a rcfg, that means change that
instead of avp
*ravp!=0 says: there are already handlers installed in rcfg

So an equal but shorter version would be

      case MP_HANDLER_ACTION_PUSH:
        if (ravp) {
            if (!*ravp) {
  	        /* initialize ravp either from avp or as an empty
array */
                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);
        }
        break;

or

      case MP_HANDLER_ACTION_PUSH:
        if (ravp && !*ravp) {
	    /* initialize ravp either from avp or as an empty array
*/
            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 (!ravp && !*avp) {
            /* directly modify the configuration at startup
time */
            *avp = modperl_handler_array_new(p);
        }
        break;

The point is if ravp!=0 we must not change *avp.

Torsten