Improve mag_conn memory handling

Create a pool just for the mag_conn structure, so that we can clear up
all the memory used when a reset is necessary.
This also fixes a segfault introduced by a previous patch where we mistakenly
zeroed the whole structure including the memory pool pointer, which needs to
be preserved.

Closes #40

Signed-off-by: Simo Sorce <simo@redhat.com>

diff --git a/src/mod_auth_gssapi.c b/src/mod_auth_gssapi.c
index 911ac92..24cee3d 100644 (file)
--- a/src/mod_auth_gssapi.c
+++ b/src/mod_auth_gssapi.c
@@ -96,9 +96,7 @@ static int mag_pre_connection(conn_rec *c, void *csd)
 {
     struct mag_conn *mc;
 
-    mc = apr_pcalloc(c->pool, sizeof(struct mag_conn));
-
-    mc->parent = c->pool;
+    mc = mag_new_conn_ctx(c->pool);
     ap_set_module_config(c->conn_config, &auth_gssapi_module, (void*)mc);
     return OK;
 }
@@ -111,10 +109,34 @@ static apr_status_t mag_conn_destroy(void *ptr)
     if (mc->ctx) {
         (void)gss_delete_sec_context(&min, &mc->ctx, GSS_C_NO_BUFFER);
     }
-    memset(mc, 0, sizeof(struct mag_conn));
     return APR_SUCCESS;
 }
 
+struct mag_conn *mag_new_conn_ctx(apr_pool_t *pool)
+{
+    struct mag_conn *mc;
+
+    mc = apr_pcalloc(pool, sizeof(struct mag_conn));
+    apr_pool_create(&mc->pool, pool);
+    /* register the context in the memory pool, so it can be freed
+     * when the connection/request is terminated */
+    apr_pool_cleanup_register(mc->pool, (void *)mc,
+                              mag_conn_destroy, apr_pool_cleanup_null);
+
+    return mc;
+}
+
+static void mag_conn_clear(struct mag_conn *mc)
+{
+    (void)mag_conn_destroy(mc);
+    apr_pool_t *temp;
+
+    apr_pool_clear(mc->pool);
+    temp = mc->pool;
+    memset(mc, 0, sizeof(struct mag_conn));
+    mc->pool = temp;
+}
+
 static bool mag_conn_is_https(conn_rec *c)
 {
     if (mag_is_https) {
@@ -422,11 +444,6 @@ static int mag_auth(request_rec *req)
     auth_header = apr_table_get(req->headers_in, "Authorization");
 
     if (mc) {
-        /* register the context in the memory pool, so it can be freed
-         * when the connection/request is terminated */
-        apr_pool_cleanup_register(mc->parent, (void *) mc,
-                                  mag_conn_destroy, apr_pool_cleanup_null);
-
         if (mc->established && !auth_header) {
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, req,
                           "Already established context found!");
@@ -456,7 +473,7 @@ static int mag_auth(request_rec *req)
         /* if we are re-authenticating make sure the conn context
          * is cleaned up so we do not accidentally reuse an existing
          * established context */
-        mag_conn_destroy(mc);
+        mag_conn_clear(mc);
     }
 
     switch (auth_type) {
@@ -492,7 +509,7 @@ static int mag_auth(request_rec *req)
                 ret = OK;
                 goto done;
             } else {
-                mag_conn_destroy(mc);
+                mag_conn_clear(mc);
             }
         }
 
@@ -720,8 +737,8 @@ static int mag_auth(request_rec *req)
     }
 
     if (mc) {
-        mc->user_name = apr_pstrdup(mc->parent, req->user);
-        mc->gss_name = apr_pstrdup(mc->parent, clientname);
+        mc->user_name = apr_pstrdup(mc->pool, req->user);
+        mc->gss_name = apr_pstrdup(mc->pool, clientname);
         mc->established = true;
         if (vtime == GSS_C_INDEFINITE || vtime < MIN_SESS_EXP_TIME) {
             vtime = MIN_SESS_EXP_TIME;

diff --git a/src/mod_auth_gssapi.h b/src/mod_auth_gssapi.h
index 00765c4..b606803 100644 (file)
--- a/src/mod_auth_gssapi.h
+++ b/src/mod_auth_gssapi.h
@@ -60,7 +60,7 @@ struct mag_config {
 };
 
 struct mag_conn {
-    apr_pool_t *parent;
+    apr_pool_t *pool;
     gss_ctx_id_t ctx;
     bool established;
     const char *user_name;
@@ -72,3 +72,5 @@ struct mag_conn {
 };
 
 #define discard_const(ptr) ((void *)((uintptr_t)(ptr)))
+
+struct mag_conn *mag_new_conn_ctx(apr_pool_t *pool);

diff --git a/src/sessions.c b/src/sessions.c
index 20679f9..73d600c 100644 (file)
--- a/src/sessions.c
+++ b/src/sessions.c
@@ -108,11 +108,7 @@ void mag_check_session(request_rec *req,
 
     mc = *conn;
     if (!mc) {
-        mc = apr_pcalloc(req->pool, sizeof(struct mag_conn));
-        if (!mc) return;
-
-        mc->parent = req->pool;
-        *conn = mc;
+        *conn = mc = mag_new_conn_ctx(req->pool);
     }
 
     rc = mag_session_get(req, sess, MAG_BEARER_KEY, &sessval);
@@ -165,19 +161,19 @@ void mag_check_session(request_rec *req,
     }
 
     /* user name */
-    mc->user_name = apr_pstrndup(mc->parent,
+    mc->user_name = apr_pstrndup(mc->pool,
                                  (char *)gsessdata->username.buf,
                                  gsessdata->username.size);
     if (!mc->user_name) goto done;
 
     /* gssapi name */
-    mc->gss_name = apr_pstrndup(mc->parent,
+    mc->gss_name = apr_pstrndup(mc->pool,
                                 (char *)gsessdata->gssname.buf,
                                 gsessdata->gssname.size);
     if (!mc->gss_name) goto done;
 
     mc->basic_hash.length = gsessdata->basichash.size;
-    mc->basic_hash.value = apr_palloc(mc->parent, mc->basic_hash.length);
+    mc->basic_hash.value = apr_palloc(mc->pool, mc->basic_hash.length);
     memcpy(mc->basic_hash.value,
            gsessdata->basichash.buf, gsessdata->basichash.size);
 
@@ -335,6 +331,6 @@ void mag_basic_cache(struct mag_config *cfg, struct mag_conn *mc,
     if (ret != 0) return;
 
     mc->basic_hash.length = mac_size;
-    mc->basic_hash.value = apr_palloc(mc->parent, mac_size);
+    mc->basic_hash.value = apr_palloc(mc->pool, mac_size);
     memcpy(mc->basic_hash.value, mac, mac_size);
 }