From 749922e90317c04b24b2df38657dbe7dd09e1bd8 Mon Sep 17 00:00:00 2001
From: Stefan Eissing <icing@apache.org>
Date: Thu, 19 Jan 2017 12:57:08 +0000
Subject: [PATCH] On the trunk:

  *) mod_http2: change lifetime of h2_session record to address crashes
     during connection shutdown. [Yann Ylavic, Stefan Eissing]



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1779459 13f79535-47bb-0310-9956-ffa450edef68
---
 CHANGES                       |  3 +++
 modules/http2/h2_bucket_eoc.c | 26 ++------------------------
 modules/http2/h2_session.c    | 35 ++++++++++++++++++++++-------------
 3 files changed, 27 insertions(+), 37 deletions(-)

diff --git a/CHANGES b/CHANGES
index ef0a770bfa..b609bfb9fa 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_http2: change lifetime of h2_session record to address crashes
+     during connection shutdown. [Yann Ylavic, Stefan Eissing]
+     
   *) mod_proxy_fcgi: Return to 2.4.20-and-earlier behavior of leaving 
      a "proxy:fcgi://" prefix in the SCRIPT_FILENAME environment variable by 
      default.  Add ProxyFCGIBackendType to allow the type of backend to be
diff --git a/modules/http2/h2_bucket_eoc.c b/modules/http2/h2_bucket_eoc.c
index 33144ef50b..cdbacae152 100644
--- a/modules/http2/h2_bucket_eoc.c
+++ b/modules/http2/h2_bucket_eoc.c
@@ -33,20 +33,6 @@ typedef struct {
     h2_session *session;
 } h2_bucket_eoc;
 
-static apr_status_t bucket_cleanup(void *data)
-{
-    h2_session **psession = data;
-
-    if (*psession) {
-        /*
-         * If bucket_destroy is called after us, this prevents
-         * bucket_destroy from trying to destroy the pool again.
-         */
-        *psession = NULL;
-    }
-    return APR_SUCCESS;
-}
-
 static apr_status_t bucket_read(apr_bucket *b, const char **str,
                                 apr_size_t *len, apr_read_type_e block)
 {
@@ -77,12 +63,7 @@ apr_bucket * h2_bucket_eoc_create(apr_bucket_alloc_t *list, h2_session *session)
     APR_BUCKET_INIT(b);
     b->free = apr_bucket_free;
     b->list = list;
-    b = h2_bucket_eoc_make(b, session);
-    if (session) {
-        h2_bucket_eoc *h = b->data;
-        apr_pool_pre_cleanup_register(session->pool, &h->session, bucket_cleanup);
-    }
-    return b;
+    return h2_bucket_eoc_make(b, session);
 }
 
 static void bucket_destroy(void *data)
@@ -92,10 +73,7 @@ static void bucket_destroy(void *data)
     if (apr_bucket_shared_destroy(h)) {
         h2_session *session = h->session;
         apr_bucket_free(h);
-        if (session) {
-            h2_session_eoc_callback(session);
-            /* all is gone now */
-        }
+        h2_session_eoc_callback(session);
     }
 }
 
diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 93cfe33718..c977fffc02 100644
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -730,7 +730,7 @@ static apr_status_t init_callbacks(conn_rec *c, nghttp2_session_callbacks **pcb)
     return APR_SUCCESS;
 }
 
-static void h2_session_destroy(h2_session *session)
+static void h2_session_cleanup(h2_session *session)
 {
     ap_assert(session);    
 
@@ -752,13 +752,17 @@ static void h2_session_destroy(h2_session *session)
 
     if (APLOGctrace1(session->c)) {
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
-                      "h2_session(%ld): destroy", session->id);
-    }
-    if (session->pool) {
-        apr_pool_destroy(session->pool);
+                      "h2_session(%ld): cleanup", session->id);
     }
 }
 
+static void h2_session_destroy(h2_session *session)
+{
+    apr_pool_t *p = session->pool;
+    session->pool = NULL;
+    apr_pool_destroy(p);
+}
+
 static apr_status_t h2_session_shutdown_notice(h2_session *session)
 {
     apr_status_t status;
@@ -857,9 +861,8 @@ static apr_status_t session_pool_cleanup(void *data)
                       "goodbye, clients will be confused, should not happen", 
                       session->id);
     }
-    /* keep us from destroying the pool, since that is already ongoing. */
+    h2_session_cleanup(session);
     session->pool = NULL;
-    h2_session_destroy(session);
     return APR_SUCCESS;
 }
 
@@ -918,7 +921,9 @@ static h2_session *h2_session_create_int(conn_rec *c,
     }
     apr_pool_tag(pool, "h2_session");
 
-    session = apr_pcalloc(pool, sizeof(h2_session));
+    /* get h2_session a lifetime beyond its pool and everything
+     * connected to it. */
+    session = apr_pcalloc(c->pool, sizeof(h2_session));
     if (session) {
         int rv;
         nghttp2_mem *mem;
@@ -1007,7 +1012,7 @@ static h2_session *h2_session_create_int(conn_rec *c,
             h2_session_destroy(session);
             return NULL;
         }
-         
+        
         n = h2_config_geti(session->config, H2_CONF_PUSH_DIARY_SIZE);
         session->push_diary = h2_push_diary_create(session->pool, n);
         
@@ -1039,10 +1044,14 @@ h2_session *h2_session_rcreate(request_rec *r, h2_ctx *ctx, h2_workers *workers)
 
 void h2_session_eoc_callback(h2_session *session)
 {
-    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
-                  "session(%ld): cleanup and destroy", session->id);
-    apr_pool_cleanup_kill(session->pool, session, session_pool_cleanup);
-    h2_session_destroy(session);
+    /* keep us from destroying the pool, if it's already done (cleanup). */
+    if (session->pool) {
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
+                      "session(%ld): cleanup and destroy", session->id);
+        apr_pool_cleanup_kill(session->pool, session, session_pool_cleanup);
+        h2_session_cleanup(session);
+        h2_session_destroy(session);
+    }
 }
 
 static apr_status_t h2_session_start(h2_session *session, int *rv)
-- 
2.40.0