From 40fc1ca7b9b8442d0bd7d15044c9d9e766eee268 Mon Sep 17 00:00:00 2001
From: Stefan Eissing <icing@apache.org>
Date: Wed, 15 Jun 2016 09:30:14 +0000
Subject: [PATCH] mod_http2: more rigid error handling in DATA frame assembly

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1748531 13f79535-47bb-0310-9956-ffa450edef68
---
 CHANGES                    |  4 ++++
 modules/http2/h2_conn_io.c |  2 +-
 modules/http2/h2_session.c | 46 ++++++++++++++++++++++++--------------
 modules/http2/h2_version.h |  4 ++--
 4 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/CHANGES b/CHANGES
index 861e79bab4..da85689ce8 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_http2: more rigid error handling in DATA frame assembly, leading
+     to deterministic connection errors if assembly fails.
+     [Stefan Eissing, Pal Nilsen <https://github.com/maedox>]
+     
   *) core: Drop an invalid Last-Modified header value coming
      from a FCGI/CGI script instead of replacing it with Unix epoch.
      [Luca Toscano]
diff --git a/modules/http2/h2_conn_io.c b/modules/http2/h2_conn_io.c
index fb679ad3de..f861536370 100644
--- a/modules/http2/h2_conn_io.c
+++ b/modules/http2/h2_conn_io.c
@@ -394,7 +394,7 @@ apr_status_t h2_conn_io_pass(h2_conn_io *io, apr_bucket_brigade *bb)
             }
             else {
                 /* bucket fits in remain, copy to scratch */
-                read_to_scratch(io, b);
+                status = read_to_scratch(io, b);
                 apr_bucket_delete(b);
                 continue;
             }
diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index f2698c48b3..2d4e0a41e7 100644
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -569,6 +569,7 @@ static int on_send_data_cb(nghttp2_session *ngh2,
     int eos;
     h2_stream *stream;
     apr_bucket *b;
+    apr_off_t len = length;
     
     (void)ngh2;
     (void)source;
@@ -577,41 +578,53 @@ static int on_send_data_cb(nghttp2_session *ngh2,
     }
     padlen = (unsigned char)frame->data.padlen;
     
+    ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c,
+                  "h2_stream(%ld-%d): send_data_cb for %ld bytes",
+                  session->id, (int)stream_id, (long)length);
+                  
     stream = get_stream(session, stream_id);
     if (!stream) {
         ap_log_cerror(APLOG_MARK, APLOG_ERR, APR_NOTFOUND, session->c,
                       APLOGNO(02924) 
-                      "h2_stream(%ld-%d): send_data",
+                      "h2_stream(%ld-%d): send_data, lookup stream",
                       session->id, (int)stream_id);
         return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     
-    ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c,
-                  "h2_stream(%ld-%d): send_data_cb for %ld bytes",
-                  session->id, (int)stream_id, (long)length);
-                  
     status = h2_conn_io_write(&session->io, (const char *)framehd, 9);
     if (padlen && status == APR_SUCCESS) {
         status = h2_conn_io_write(&session->io, (const char *)&padlen, 1);
     }
     
-    if (status == APR_SUCCESS) {
-        apr_off_t len = length;
-        status = h2_stream_read_to(stream, session->bbtmp, &len, &eos);
-        if (status == APR_SUCCESS && len != length) {
-            status = APR_EINVAL;
-        }
+    if (status != APR_SUCCESS) {
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c,
+                      "h2_stream(%ld-%d): writing frame header",
+                      session->id, (int)stream_id);
+        return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     
-    if (status == APR_SUCCESS && padlen) {
+    status = h2_stream_read_to(stream, session->bbtmp, &len, &eos);
+    if (status != APR_SUCCESS) {
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c,
+                      "h2_stream(%ld-%d): send_data_cb, reading stream",
+                      session->id, (int)stream_id);
+        return NGHTTP2_ERR_CALLBACK_FAILURE;
+    }
+    else if (len != length) {
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c,
+                      "h2_stream(%ld-%d): send_data_cb, wanted %ld bytes, "
+                      "got %ld from stream",
+                      session->id, (int)stream_id, (long)length, (long)len);
+        return NGHTTP2_ERR_CALLBACK_FAILURE;
+    }
+    
+    if (padlen) {
         b = apr_bucket_immortal_create(immortal_zeros, padlen, 
                                        session->c->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(session->bbtmp, b);
     }
     
-    if (status == APR_SUCCESS) {
-        status = h2_conn_io_pass(&session->io, session->bbtmp);
-    }
+    status = h2_conn_io_pass(&session->io, session->bbtmp);
         
     apr_brigade_cleanup(session->bbtmp);
     if (status == APR_SUCCESS) {
@@ -623,9 +636,8 @@ static int on_send_data_cb(nghttp2_session *ngh2,
                       APLOGNO(02925) 
                       "h2_stream(%ld-%d): failed send_data_cb",
                       session->id, (int)stream_id);
+        return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
-    
-    return h2_session_status_from_apr_status(status);
 }
 
 static int on_frame_send_cb(nghttp2_session *ngh2, 
diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index 6245f27c2b..42e18bd375 100644
--- a/modules/http2/h2_version.h
+++ b/modules/http2/h2_version.h
@@ -26,7 +26,7 @@
  * @macro
  * Version number of the http2 module as c string
  */
-#define MOD_HTTP2_VERSION "1.5.10-DEV"
+#define MOD_HTTP2_VERSION "1.5.11-DEV"
 
 /**
  * @macro
@@ -34,7 +34,7 @@
  * release. This is a 24 bit number with 8 bits for major number, 8 bits
  * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
  */
-#define MOD_HTTP2_VERSION_NUM 0x01050a
+#define MOD_HTTP2_VERSION_NUM 0x01050b
 
 
 #endif /* mod_h2_h2_version_h */
-- 
2.50.1