git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: svn commit: r1841225 - /httpd/httpd/trunk/modules/dav/main/props.c


On Tue, Sep 18, 2018 at 03:04:13PM +0200, Ruediger Pluem wrote:
> Pools are very tricky in mod_dav. Hence additional eyeballs are very much welcome here.
> As I only did testing with mod_dav_fs I would be keen to know if things still work with Subversion.
> So if someone from the Subversion guys is listening here: Having this tested with Subversion would be very welcome :-).

Not reviewed sorry, but I saw a crash under APR with --enable-pool-debug 
when running t/modules/dav.t - the backtrace has the magic 0x41s which 
suggests pool-related:

#0  0x00007f74e8deb7c1 in __strlen_avx2 () from /lib64/libc.so.6
#1  0x00007f74d3901093 in dav_send_one_response 
(response=0x7f747cff8650, bb=0x7f746c01f960, r=0x7f746c002230, 
    pool=0x7f746c01f3e0) at mod_dav.c:491
#2  0x00007f74d39012e6 in dav_stream_response (status=0, 
wres=0x7f747cff8808, wres=0x7f747cff8808, 
    pool=<optimized out>, propstats=<synthetic pointer>) at 
mod_dav.c:1192
#3  dav_propfind_walker (wres=0x7f747cff8808, calltype=<optimized out>) 
at mod_dav.c:2064
#4  0x00007f74d28c7f8c in dav_fs_walker 
(fsctx=fsctx@entry=0x7f747cff8800, depth=depth@entry=1) at repos.c:1759
#5  0x00007f74d28c852f in dav_fs_internal_walk (params=0x7f747cff8ad0, 
depth=1, is_move=0, root_dst=0x0, 
    response=0x7f747cff8ac8) at repos.c:1844
#6  0x00007f74d3905b3e in dav_method_propfind (r=0x7f746c002230) at 
mod_dav.c:2190

...
(gdb) up
#1  0x00007f74d3901093 in dav_send_one_response 
(response=0x7f747cff8650, bb=0x7f746c01f960, r=0x7f746c002230, 
    pool=0x7f746c01f3e0) at mod_dav.c:491
491	        ap_fputs(r->output_filters, bb, t->text);
(gdb) print t
$1 = (apr_text *) 0x7f746c01dcc0
(gdb) print *t
$2 = {text = 0x0, next = 0x4141414141414141}


> 
> Regards
> 
> Rüdiger
> 
> On 09/18/2018 02:58 PM, rpluem@xxxxxxxxxx wrote:
> > Author: rpluem
> > Date: Tue Sep 18 12:58:57 2018
> > New Revision: 1841225
> > 
> > URL: http://svn.apache.org/viewvc?rev=1841225&view=rev
> > Log:
> > * Doing a PROPFIND on a large collection e.g. 50.000 elements can easily
> >   consume 1 GB of memory as the subrequests and propdb pools are not
> >   destroyed and cleared after each element was handled.
> >   Do this now. There is one case in dav_get_props where elem->priv
> >   lives longer then the propdb pool. In this case allocate from r->pool.
> >   Furthermore also recycle propdb's which allows to clear the propdb's
> >   pools instead of destroying them and creating them again.
> > 
> > Modified:
> >     httpd/httpd/trunk/modules/dav/main/props.c
> > 
> > Modified: httpd/httpd/trunk/modules/dav/main/props.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/props.c?rev=1841225&r1=1841224&r2=1841225&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/modules/dav/main/props.c (original)
> > +++ httpd/httpd/trunk/modules/dav/main/props.c Tue Sep 18 12:58:57 2018
> > @@ -524,7 +524,21 @@ DAV_DECLARE(dav_error *)dav_open_propdb(
> >                                          apr_array_header_t * ns_xlate,
> >                                          dav_propdb **p_propdb)
> >  {
> > -    dav_propdb *propdb = apr_pcalloc(r->pool, sizeof(*propdb));
> > +    dav_propdb *propdb = NULL;
> > +    /*
> > +     * Check if we have tucked away a previous propdb and reuse it.
> > +     * Otherwise create a new one and tuck it away
> > +     */
> > +    apr_pool_userdata_get((void **)&propdb, "propdb", r->pool);
> > +    if (!propdb) {
> > +        propdb = apr_pcalloc(r->pool, sizeof(*propdb));
> > +        apr_pool_userdata_setn(propdb, "propdb", NULL, r->pool);
> > +        apr_pool_create(&propdb->p, r->pool);
> > +    }
> > +    else {
> > +        /* Play safe and clear the pool of the reused probdb */
> > +        apr_pool_clear(propdb->p);
> > +    }
> >  
> >      *p_propdb = NULL;
> >  
> > @@ -537,7 +551,6 @@ DAV_DECLARE(dav_error *)dav_open_propdb(
> >  #endif
> >  
> >      propdb->r = r;
> > -    apr_pool_create(&propdb->p, r->pool);
> >      propdb->resource = resource;
> >      propdb->ns_xlate = ns_xlate;
> >  
> > @@ -562,10 +575,12 @@ DAV_DECLARE(void) dav_close_propdb(dav_p
> >          (*propdb->db_hooks->close)(propdb->db);
> >      }
> >  
> > -    /* Currently, mod_dav's pool usage doesn't allow clearing this pool. */
> > -#if 0
> > -    apr_pool_destroy(propdb->p);
> > -#endif
> > +    if (propdb->subreq) {
> > +        ap_destroy_sub_req(propdb->subreq);
> > +        propdb->subreq = NULL;
> > +    }
> > +
> > +    apr_pool_clear(propdb->p);
> >  }
> >  
> >  DAV_DECLARE(dav_get_props_result) dav_get_allprops(dav_propdb *propdb,
> > @@ -815,7 +830,8 @@ DAV_DECLARE(dav_get_props_result) dav_ge
> >          */
> >  
> >          if (elem->priv == NULL) {
> > -            elem->priv = apr_pcalloc(propdb->p, sizeof(*priv));
> > +            /* elem->priv outlives propdb->p. Hence use the request pool */
> > +            elem->priv = apr_pcalloc(propdb->r->pool, sizeof(*priv));
> >          }
> >          priv = elem->priv;
> >  
> > 
> > 
> >