On Tue, Jun 25, 2019 at 09:06:42AM +0000, Plüm, Rüdiger, Vodafone Group wrote:
> > On Tue, Jun 25, 2019 at 08:49:08AM +0100, Joe Orton wrote:
> > > Unless I am missing something the apr_dir_open/apr_dir_read API design
> > > is always going to have memory use proportional to directory size
> > > because apr_dir_read() duplicates the filename into the directory's
> > > pool. We need an apr_dir_pread() or whatever which takes a pool
> > > argument. :(
> >
> > OK, yes, this plus an iterpool in dav_fs_walker() fixes it.
>
> Only the iterpool in dav_fs_walker or plus the changes in APR (apr_dir_pread)?
Yes, both - PoC attached but I'm not at all confident this is safe, esp
in the recursion case.
The test I was running before was only a "core" liveprop provided by
mod_dav itself, so I also tested the deadprops case and that still seems
to leak. sbrk() to increase the heap is getting triggered by
apr_sdbm_open() every time I saw it, but apr_dbm_open() is being pass
the scratchpool so I think it's not the cause.
Running "dump_all_pools propdb->p" at this point gives me:
Pool 'transaction' [0x24e1d88]: 7264/8192 free (1 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'master_conn' [0x2504e28]: 1224/8192 free (1 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'deferred_pool' [0x263ef18]: 8032/8192 free (1 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'request' [0x2506e38]: 13672/1646592 free (201 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
Pool 'mod_dav_fs-walker' [0x2524648]: 8016/8192 free (1 blocks)
allocator: 0x24d2b90 free blocks in allocator: 0 kiB
Pool 'mod_dav-scratch' [0x2508e48]: 7224/8192 free (1 blocks)
allocator: 0x24d2b90 free blocks in allocator: 0 kiB
Pool 'mod_lua-vm' [0x250ce68]: 6360/16384 free (2 blocks) allocator:
0x24d2b90 free blocks in allocator: 0 kiB
[trimmed some whitespace only]
I think this is implicating misuse of r->pool, that's grown rather
large?
Regards, Joe
Index: include/apr_file_info.h
===================================================================
--- include/apr_file_info.h (revision 1862036)
+++ include/apr_file_info.h (working copy)
@@ -268,6 +268,24 @@
apr_dir_t *thedir);
/**
+ * Read the next entry from the specified directory.
+ * @param finfo the file info structure and filled in by apr_dir_read
+ * @param wanted The desired apr_finfo_t fields, as a bit flag of APR_FINFO_
+ values
+ * @param thedir the directory descriptor returned from apr_dir_open
+ * @param pool the pool to use for allocations
+ * @remark No ordering is guaranteed for the entries read.
+ *
+ * @note If @c APR_INCOMPLETE is returned all the fields in @a finfo may
+ * not be filled in, and you need to check the @c finfo->valid bitmask
+ * to verify that what you're looking for is there. When no more
+ * entries are available, APR_ENOENT is returned.
+ */
+APR_DECLARE(apr_status_t) apr_dir_pread(apr_finfo_t *finfo, apr_int32_t wanted,
+ apr_dir_t *thedir, apr_pool_t *pool);
+
+
+/**
* Rewind the directory to the first entry.
* @param thedir the directory descriptor to rewind.
*/
Index: file_io/unix/dir.c
===================================================================
--- file_io/unix/dir.c (revision 1862036)
+++ file_io/unix/dir.c (working copy)
@@ -133,6 +133,12 @@
apr_status_t apr_dir_read(apr_finfo_t *finfo, apr_int32_t wanted,
apr_dir_t *thedir)
{
+ return apr_dir_pread(finfo, wanted, thedir, thedir->pool);
+}
+
+apr_status_t apr_dir_pread(apr_finfo_t *finfo, apr_int32_t wanted,
+ apr_dir_t *thedir, apr_pool_t *pool)
+{
apr_status_t ret = 0;
#ifdef DIRENT_TYPE
apr_filetype_e type;
@@ -242,7 +248,7 @@
apr_cpystrn(end, thedir->entry->d_name,
sizeof fspec - (end - fspec));
- ret = apr_stat(finfo, fspec, APR_FINFO_LINK | wanted, thedir->pool);
+ ret = apr_stat(finfo, fspec, APR_FINFO_LINK | wanted, pool);
/* We passed a stack name that will disappear */
finfo->fname = NULL;
}
@@ -254,7 +260,7 @@
/* We don't bail because we fail to stat, when we are only -required-
* to readdir... but the result will be APR_INCOMPLETE
*/
- finfo->pool = thedir->pool;
+ finfo->pool = pool;
finfo->valid = 0;
#ifdef DIRENT_TYPE
if (type != APR_UNKFILE) {
@@ -270,7 +276,7 @@
#endif
}
- finfo->name = apr_pstrdup(thedir->pool, thedir->entry->d_name);
+ finfo->name = apr_pstrdup(pool, thedir->entry->d_name);
finfo->valid |= APR_FINFO_NAME;
if (wanted)
Index: modules/dav/fs/repos.c
===================================================================
--- modules/dav/fs/repos.c (revision 1862052)
+++ modules/dav/fs/repos.c (working copy)
@@ -1490,6 +1490,7 @@
{
const dav_walk_params *params = fsctx->params;
apr_pool_t *pool = params->pool;
+ apr_pool_t *iterpool;
apr_status_t status;
dav_error *err = NULL;
int isdir = fsctx->res1.collection;
@@ -1509,6 +1510,9 @@
return NULL;
}
+ apr_pool_create(&iterpool, pool);
+ apr_pool_tag(iterpool, "mod_dav_fs-walker");
+
/* put a trailing slash onto the directory, in preparation for appending
* files to it as we discovery them within the directory */
dav_check_bufsize(pool, &fsctx->path1, DAV_BUFFER_PAD);
@@ -1536,9 +1540,14 @@
/* ### need a better error */
return dav_new_error(pool, HTTP_NOT_FOUND, 0, status, NULL);
}
- while ((apr_dir_read(&dirent, APR_FINFO_DIRENT, dirp)) == APR_SUCCESS) {
+
+ while (1) {
apr_size_t len;
+ apr_pool_clear(iterpool);
+ status = apr_dir_pread(&dirent, APR_FINFO_DIRENT, dirp, iterpool);
+ if (status)
+ break;
len = strlen(dirent.name);
/* avoid recursing into our current, parent, or state directories */
@@ -1570,7 +1579,7 @@
dav_buffer_place_mem(pool, &fsctx->path1, dirent.name, len + 1, 0);
status = apr_stat(&fsctx->info1.finfo, fsctx->path1.buf,
- DAV_FINFO_MASK, pool);
+ DAV_FINFO_MASK, iterpool);
if (status != APR_SUCCESS && status != APR_INCOMPLETE) {
/* woah! where'd it go? */
/* ### should have a better error here */
@@ -1642,7 +1651,8 @@
/* ### check the return value of this? */
apr_dir_close(dirp);
-
+ apr_pool_destroy(iterpool);
+
if (err != NULL)
return err;
