From 3d26090bc7d3413d65a463d89b7f83d98b563ee0 Mon Sep 17 00:00:00 2001 From: Chris Darroch Date: Tue, 16 Jan 2007 19:36:26 +0000 Subject: [PATCH] We now create memory sub-pools for each DB connection and close DB connections in a pool cleanup function. This simplifies the ap_dbd_acquire() and ap_dbd_cacquire() functions, and also stops us from leaking ap_dbd_t structures when using reslists. We ensure that prepared statements are destroyed before their DB connection is closed, in case any drivers would have problems cleaning up prepared statements after the DB connection is closed. The combination of reslists and memory pool cleanup functions was causing segfaults when child processes exited, as reported in PR 39985. To prevent this, we register dbd_destroy() as a cleanup that will execute prior to the internal cleanup function registered by apr_reslist_create(). When the reslist's memory pool is destroyed, dbd_destroy() informs dbd_destruct() not to do anything when subsequently called by the reslist's internal cleanup function. We avoid the use of s->process->pool (the global pool) since it isn't destroyed by exiting child processes in most multi-process MPMs. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@496831 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 8 ++ modules/database/mod_dbd.c | 150 +++++++++++++++++++++---------------- 2 files changed, 92 insertions(+), 66 deletions(-) diff --git a/CHANGES b/CHANGES index 390077fd03..897f494e45 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,14 @@ Changes with Apache 2.3.0 [Remove entries to the current 2.0 and 2.2 section below, when backported] + *) mod_dbd: Create memory sub-pools for each DB connection and close + DB connections in a pool cleanup function. Ensure prepared statements + are destroyed before DB connection is closed. When using reslists, + prevent segfaults when child processes exit, and stop memory leakage + of ap_dbd_t structures. Avoid use of global s->process->pool, which + isn't destroyed by exiting child processes in most multi-process MPMs. + PR 39985. [Chris Darroch, Nick Kew] + *) apxs: Eliminate run-time check for mod_so. PR 40653. [David M. Lee ] diff --git a/modules/database/mod_dbd.c b/modules/database/mod_dbd.c index ea5414a548..eba281c25b 100644 --- a/modules/database/mod_dbd.c +++ b/modules/database/mod_dbd.c @@ -59,10 +59,11 @@ typedef struct { const char *params; int persist; dbd_prepared *prepared; + apr_pool_t *pool; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; - apr_pool_t *pool; apr_reslist_t *reslist; + int destroyed; int nmin; int nkeep; int nmax; @@ -303,17 +304,22 @@ static apr_status_t dbd_prepared_init(apr_pool_t *pool, svr_cfg *svr, static apr_status_t dbd_close(void *data) { ap_dbd_t *rec = data; - apr_status_t rv = apr_dbd_close(rec->driver, rec->handle); - - apr_pool_destroy(rec->pool); - return rv; + return apr_dbd_close(rec->driver, rec->handle); } #if APR_HAS_THREADS static apr_status_t dbd_destruct(void *data, void *params, apr_pool_t *pool) { - return dbd_close(data); + svr_cfg *svr = params; + + if (!svr->destroyed) { + ap_dbd_t *rec = data; + + apr_pool_destroy(rec->pool); + } + + return APR_SUCCESS; } #endif @@ -325,17 +331,21 @@ static apr_status_t dbd_construct(void **data_ptr, void *params, apr_pool_t *pool) { svr_cfg *svr = params; - ap_dbd_t *rec = apr_pcalloc(pool, sizeof(ap_dbd_t)); + apr_pool_t *rec_pool, *prepared_pool; + ap_dbd_t *rec; apr_status_t rv; - /* this pool is mostly so dbd_close can destroy the prepared stmts */ - rv = apr_pool_create(&rec->pool, pool); + rv = apr_pool_create(&rec_pool, pool); if (rv != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_CRIT, rv, svr->server, "DBD: Failed to create memory pool"); return rv; } + rec = apr_pcalloc(rec_pool, sizeof(ap_dbd_t)); + + rec->pool = rec_pool; + /* The driver is loaded at config time now, so this just checks a hash. * If that changes, the driver DSO could be registered to unload against * our pool, which is probably not what we want. Error checking isn't @@ -384,14 +394,28 @@ static apr_status_t dbd_construct(void **data_ptr, return rv; } - rv = dbd_prepared_init(rec->pool, svr, rec); + apr_pool_cleanup_register(rec->pool, rec, dbd_close, + apr_pool_cleanup_null); + + /* we use a sub-pool for the prepared statements for each connection so + * that they will be cleaned up first, before the connection is closed + */ + rv = apr_pool_create(&prepared_pool, rec->pool); + if (rv != APR_SUCCESS) { + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, svr->server, + "DBD: Failed to create memory pool"); + + apr_pool_destroy(rec->pool); + return rv; + } + + rv = dbd_prepared_init(prepared_pool, svr, rec); if (rv != APR_SUCCESS) { const char *errmsg = apr_dbd_error(rec->driver, rec->handle, rv); ap_log_error(APLOG_MARK, APLOG_ERR, rv, svr->server, "DBD: failed to prepare SQL statements: %s", (errmsg ? errmsg : "[???]")); - apr_dbd_close(rec->driver, rec->handle); apr_pool_destroy(rec->pool); return rv; } @@ -402,24 +426,37 @@ static apr_status_t dbd_construct(void **data_ptr, } #if APR_HAS_THREADS -static apr_status_t dbd_setup(apr_pool_t *pool, server_rec *s, - svr_cfg *svr) +static apr_status_t dbd_destroy(void *data) +{ + svr_cfg *svr = data; + + svr->destroyed = 1; + + return APR_SUCCESS; +} + +static apr_status_t dbd_setup(server_rec *s, svr_cfg *svr) { apr_status_t rv; - /* create a pool just for the reslist from a process-lifetime pool; - * that pool (s->process->pool in the dbd_setup_lock case, - * whatever was passed to ap_run_child_init in the dbd_setup_init case) - * will be shared with other threads doing other non-mod_dbd things - * so we can't use it for the reslist directly + /* We create the reslist using a sub-pool of the pool passed to our + * child_init hook. No other threads can be here because we're + * either in the child_init phase or dbd_setup_lock() acquired our mutex. + * No other threads will use this sub-pool after this, except via + * reslist calls, which have an internal mutex. + * + * We need to short-circuit the cleanup registered internally by + * apr_reslist_create(). We do this by registering dbd_destroy() + * as a cleanup afterwards, so that it will run before the reslist's + * internal cleanup. + * + * If we didn't do this, then we could free memory twice when the pool + * was destroyed. When apr_pool_destroy() runs, it first destroys all + * all the per-connection sub-pools created in dbd_construct(), and + * then it runs the reslist's cleanup. The cleanup calls dbd_destruct() + * on each resource, which would then attempt to destroy the sub-pools + * a second time. */ - rv = apr_pool_create(&svr->pool, pool); - if (rv != APR_SUCCESS) { - ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, - "DBD: Failed to create reslist memory pool"); - return rv; - } - rv = apr_reslist_create(&svr->reslist, svr->nmin, svr->nkeep, svr->nmax, apr_time_from_sec(svr->exptime), @@ -428,12 +465,15 @@ static apr_status_t dbd_setup(apr_pool_t *pool, server_rec *s, if (rv != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, "DBD: failed to initialise"); - apr_pool_destroy(svr->pool); return rv; } + apr_pool_cleanup_register(svr->pool, svr, dbd_destroy, + apr_pool_cleanup_null); + return APR_SUCCESS; } +#endif static apr_status_t dbd_setup_init(apr_pool_t *pool, server_rec *s) { @@ -453,7 +493,15 @@ static apr_status_t dbd_setup_init(apr_pool_t *pool, server_rec *s) return APR_SUCCESS; } - rv = dbd_setup(pool, s, svr); + rv = apr_pool_create(&svr->pool, pool); + if (rv != APR_SUCCESS) { + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, + "DBD: Failed to create reslist cleanup memory pool"); + return rv; + } + +#if APR_HAS_THREADS + rv = dbd_setup(s, svr); if (rv == APR_SUCCESS) { return rv; } @@ -467,12 +515,13 @@ static apr_status_t dbd_setup_init(apr_pool_t *pool, server_rec *s) ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, "DBD: Failed to create thread mutex"); } +#endif return rv; } -static apr_status_t dbd_setup_lock(apr_pool_t *pool, server_rec *s, - svr_cfg *svr) +#if APR_HAS_THREADS +static apr_status_t dbd_setup_lock(server_rec *s, svr_cfg *svr) { apr_status_t rv = APR_SUCCESS, rv2; @@ -492,7 +541,7 @@ static apr_status_t dbd_setup_lock(apr_pool_t *pool, server_rec *s, } if (!svr->reslist) { - rv = dbd_setup(s->process->pool, s, svr); + rv = dbd_setup(s, svr); } rv2 = apr_thread_mutex_unlock(svr->mutex); @@ -517,7 +566,7 @@ DBD_DECLARE_NONSTD(void) ap_dbd_close(server_rec *s, ap_dbd_t *rec) svr_cfg *svr = ap_get_module_config(s->module_config, &dbd_module); if (!svr->persist) { - dbd_close((void*) rec); + apr_pool_destroy(rec->pool); } #if APR_HAS_THREADS else { @@ -563,13 +612,13 @@ DBD_DECLARE_NONSTD(ap_dbd_t*) ap_dbd_open(apr_pool_t *pool, server_rec *s) if (!svr->persist) { /* Return a once-only connection */ - dbd_construct((void*) &rec, svr, s->process->pool); + dbd_construct((void*) &rec, svr, pool); return rec; } #if APR_HAS_THREADS if (!svr->reslist) { - if (dbd_setup_lock(pool, s, svr) != APR_SUCCESS) { + if (dbd_setup_lock(s, svr) != APR_SUCCESS) { return NULL; } } @@ -592,20 +641,15 @@ DBD_DECLARE_NONSTD(ap_dbd_t*) ap_dbd_open(apr_pool_t *pool, server_rec *s) rec = svr->rec; if (rec) { if (dbd_check(pool, s, rec) != APR_SUCCESS) { - apr_pool_cleanup_run(s->process->pool, rec, dbd_close); + apr_pool_destroy(rec->pool); rec = NULL; } } /* We don't have a connection right now, so we'll open one */ if (!rec) { - dbd_construct((void*) &rec, svr, s->process->pool); + dbd_construct((void*) &rec, svr, svr->pool); svr->rec = rec; - - if (rec) { - apr_pool_cleanup_register(s->process->pool, rec, - dbd_close, apr_pool_cleanup_null); - } } #endif @@ -652,10 +696,6 @@ DBD_DECLARE_NONSTD(ap_dbd_t *) ap_dbd_acquire(request_rec *r) apr_pool_cleanup_register(r->pool, acq, dbd_release, apr_pool_cleanup_null); } - else { - apr_pool_cleanup_register(r->pool, acq->rec, dbd_close, - apr_pool_cleanup_null); - } } } @@ -679,10 +719,6 @@ DBD_DECLARE_NONSTD(ap_dbd_t *) ap_dbd_cacquire(conn_rec *c) apr_pool_cleanup_register(c->pool, acq, dbd_release, apr_pool_cleanup_null); } - else { - apr_pool_cleanup_register(c->pool, acq->rec, dbd_close, - apr_pool_cleanup_null); - } } } @@ -706,15 +742,7 @@ DBD_DECLARE_NONSTD(ap_dbd_t *) ap_dbd_acquire(request_rec *r) if (!rec) { rec = ap_dbd_open(r->pool, r->server); if (rec) { - svr_cfg *svr = ap_get_module_config(r->server->module_config, - &dbd_module); - ap_set_module_config(r->request_config, &dbd_module, rec); - /* if persist then ap_dbd_open registered cleanup on proc pool */ - if (!svr->persist) { - apr_pool_cleanup_register(r->pool, rec, dbd_close, - apr_pool_cleanup_null); - } } } @@ -728,15 +756,7 @@ DBD_DECLARE_NONSTD(ap_dbd_t *) ap_dbd_cacquire(conn_rec *c) if (!rec) { rec = ap_dbd_open(c->pool, c->base_server); if (rec) { - svr_cfg *svr = ap_get_module_config(c->base_server->module_config, - &dbd_module); - ap_set_module_config(c->conn_config, &dbd_module, rec); - /* if persist then ap_dbd_open registered cleanup on proc pool */ - if (!svr->persist) { - apr_pool_cleanup_register(c->pool, rec, dbd_close, - apr_pool_cleanup_null); - } } } @@ -748,9 +768,7 @@ static void dbd_hooks(apr_pool_t *pool) { ap_hook_pre_config(dbd_pre_config, NULL, NULL, APR_HOOK_MIDDLE); ap_hook_post_config(dbd_post_config, NULL, NULL, APR_HOOK_MIDDLE); -#if APR_HAS_THREADS ap_hook_child_init((void*)dbd_setup_init, NULL, NULL, APR_HOOK_MIDDLE); -#endif APR_REGISTER_OPTIONAL_FN(ap_dbd_prepare); APR_REGISTER_OPTIONAL_FN(ap_dbd_open); -- 2.40.0