On 1 September 2010 22:08, Jeff Trawick <traw...@gmail.com> wrote: > On Wed, Sep 1, 2010 at 6:37 AM, Graham Dumpleton > <graham.dumple...@gmail.com> wrote: >> >> On 1 September 2010 20:15, Graham Leggett <minf...@sharp.fm> wrote: >> > On 01 Sep 2010, at 6:07 AM, dave b wrote: >> > >> >> What is the rational behind not checking the return value of >> >> apr_palloc and apr_pcalloc? >> > >> > The rationale is to not be forced to check for and handle hundreds of >> > potential failure cases when you're probably doomed anyway.
Yes I agree :) Hence, why the patch I put in the bug report used assert :) >> > The APR pools API gives you the apr_pool_abort_set() function, which >> > specifies a function to call if the memory allocation fails. In the case >> > of >> > httpd, a function is registered which gracefully shuts down that >> > particular >> > server process if the allocation fails, and apr_palloc() is in the >> > process >> > guaranteed to never return NULL. >> >> Noting that apr_pool_abort_set() is only setup in Apache 2.3 and not >> in Apache 2.2.16 or earlier. Not being in 2.X explains why I couldn't >> find it before. >> >> Any reason why setting up apr_pool_abort_set() wasn't back ported to >> Apache 2.2? > > dunno, but here's the commit and original discussion in case anyone wants to > read > http://svn.apache.org/viewvc?view=revision&revision=406953 (dunno if it was > subsequently fixed) > http://www.mail-archive.com/dev@httpd.apache.org/msg32257.html Interesting patch, um but... I think there maybe a problem here: from apr :) include/apr_pools.h #if defined(DOXYGEN) APR_DECLARE(apr_status_t) apr_pool_create(apr_pool_t **newpool, apr_pool_t *parent); #else #if APR_POOL_DEBUG #define apr_pool_create(newpool, parent) \ apr_pool_create_ex_debug(newpool, parent, NULL, NULL, \ APR_POOL__FILE_LINE__) #else #define apr_pool_create(newpool, parent) \ apr_pool_create_ex(newpool, parent, NULL, NULL) #endif #endif And apr_pool_create_ex is: APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool, apr_pool_t *parent, apr_abortfunc_t abort_fn, apr_allocator_t *allocator); First thing to note is that in main apr_app_initialize[1] in server/main.c via init_process. When this happens the global_pool will not have a abort_fn. *However*, cntx will (it is set :) ), process->pconf will. Now we look in the actual code :) See [0] Right so if apr_pool_create gets called with null as the parent from now on - there will be no default abort_fn. Lets go hunting for this happens ;) see [2] Ok so lets pull out the scoreboard now ? /* ToDo: This function should be made to handle setting up * a scoreboard shared between processes using any IPC technique, * not just a shared memory segment */ static apr_status_t open_scoreboard(apr_pool_t *pconf) { #if APR_HAS_SHARED_MEMORY apr_status_t rv; char *fname = NULL; apr_pool_t *global_pool; /* We don't want to have to recreate the scoreboard after * restarts, so we'll create a global pool and never clean it. */ rv = apr_pool_create(&global_pool, NULL); MMM I wonder where else httpd does these kind of things ;) Remember my simple grep can *easily* miss some :) Ok great just get the rest of world to use apache 2.3 which has had this patch for over 4 years now. Just one problem: I don't see anyone running apache 2.3 alpha 8 on their production servers. So perhaps apache 2.2 should get a backport of this 'fix' :-) Apparently my fix that I suggested on the bug referenced in my original email is also not going to work :) (it will not be enough :-) ) Also, note I hit that code path via apr testdir (in one of the tests ;) ) - and I was not out of memory - I haven't checked which test it is yet (it would be nice if the apr tests told me this...). If the caller has hit that code path and they haven't defined an abort_fn, I think it best to exit / abort for them - If a user wants not to have that happen then they can just simply ensure they are providing their own abort_fn :) "A little less conversation, a little more action please." [0] APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool, apr_pool_t *parent, apr_abortfunc_t abort_fn, apr_allocator_t *allocator) { apr_pool_t *pool; apr_memnode_t *node; *newpool = NULL; if (!parent) parent = global_pool; /* parent will always be non-NULL here except the first time a * pool is created, in which case allocator is guaranteed to be * non-NULL. */ if (!abort_fn && parent) abort_fn = parent->abort_fn; if (allocator == NULL) allocator = parent->allocator; if ((node = allocator_alloc(allocator, MIN_ALLOC - APR_MEMNODE_T_SIZE)) == NULL) { if (abort_fn) abort_fn(APR_ENOMEM); return APR_ENOMEM; } node->next = node; node->ref = &node->next; pool = (apr_pool_t *)node->first_avail; node->first_avail = pool->self_first_avail = (char *)pool + SIZEOF_POOL_T; pool->allocator = allocator; pool->active = pool->self = node; pool->abort_fn = abort_fn; pool->child = NULL; pool->cleanups = NULL; pool->free_cleanups = NULL; pool->pre_cleanups = NULL; pool->subprocesses = NULL; pool->user_data = NULL; pool->tag = NULL; #ifdef NETWARE pool->owner_proc = (apr_os_proc_t)getnlmhandle(); #endif /* defined(NETWARE) */ if ((pool->parent = parent) != NULL) { #if APR_HAS_THREADS apr_thread_mutex_t *mutex; if ((mutex = apr_allocator_mutex_get(parent->allocator)) != NULL) apr_thread_mutex_lock(mutex); #endif /* APR_HAS_THREADS */ if ((pool->sibling = parent->child) != NULL) pool->sibling->ref = &pool->sibling; parent->child = pool; pool->ref = &parent->child; #if APR_HAS_THREADS if (mutex) apr_thread_mutex_unlock(mutex); #endif /* APR_HAS_THREADS */ } else { pool->sibling = NULL; pool->ref = NULL; } *newpool = pool; return APR_SUCCESS; } [1] static int initialized = 0; APR_DECLARE(apr_status_t) apr_initialize(void) { apr_pool_t *pool; apr_status_t status; if (initialized++) { return APR_SUCCESS; } #if !defined(BEOS) && !defined(OS2) apr_proc_mutex_unix_setup_lock(); apr_unix_setup_time(); #endif if ((status = apr_pool_initialize()) != APR_SUCCESS) return status; if (apr_pool_create(&pool, NULL) != APR_SUCCESS) { return APR_ENOPOOL; } apr_pool_tag(pool, "apr_initialize"); /* apr_atomic_init() used to be called from here aswell. * Pools rely on mutexes though, which can be backed by * atomics. Due to this circular dependency * apr_pool_initialize() is taking care of calling * apr_atomic_init() at the correct time. */ apr_signal_init(pool); return APR_SUCCESS; } [2] (obviously not all of these are hits and probably there are lots more around ;) ) grep "apr_pool_create" * -R |grep "NULL" - httpd/server/main.c: stat = apr_pool_create(&cntx, NULL); httpd/server/mpm/event/event.c: apr_pool_create_ex(&ptrans, pconf, NULL, allocator); httpd/server/mpm/simple/simple_io.c: apr_pool_create(&ptrans, NULL); httpd/server/mpm/winnt/child.c: rv = apr_pool_create_ex(&context->ptrans, pchild, NULL, httpd/server/mpm/winnt/nt_eventlog.c: apr_pool_create_ex(&p, NULL, NULL, NULL); httpd/server/mpm/winnt/mpm_winnt.c: apr_pool_create_ex(&ptemp, p, NULL, NULL); httpd/server/mpm/worker/worker.c: apr_pool_create_ex(&ptrans, pconf, NULL, allocator); httpd/server/mpm/netware/mpm_netware.c: apr_pool_create_ex(&ptrans, pmain, NULL, allocator); httpd/server/mpm/prefork/prefork.c: apr_pool_create_ex(&pchild, pconf, NULL, allocator); httpd/server/scoreboard.c: rv = apr_pool_create(&global_pool, NULL); httpd/support/ab.c: apr_pool_create(&cntxt, NULL); httpd/support/httxt2dbm.c: apr_pool_create(&pool, NULL); httpd/support/rotatelogs.c: apr_pool_create(&status.pool, NULL); httpd/support/htcacheclean.c: if (apr_pool_create(&pool, NULL) != APR_SUCCESS) { httpd/support/htdigest.c: apr_pool_create(&cntxt, NULL); httpd/support/htpasswd.c: apr_pool_create(&pool, NULL); httpd/support/htdbm.c: apr_pool_create( pool, NULL); httpd/support/fcgistarter.c: apr_pool_create(&pool, NULL); httpd/support/logresolve.c: if (apr_pool_create(&pool, NULL) != APR_SUCCESS) { httpd/modules/slotmem/mod_slotmem_shm.c: rv = apr_pool_create(&global_pool, NULL); httpd/modules/arch/win32/mod_isapi.c: apr_pool_create_ex(&loaded.pool, pconf, NULL, NULL); httpd/modules/ldap/util_ldap.c: if (apr_pool_create(&newpool, NULL) != APR_SUCCESS) { -- Every cloud engenders not a storm. -- William Shakespeare, "Henry VI"
