[gnurl] 122/411: http: consolidate nghttp2_session_mem_recv() call paths

[gnunet]

This is an automated email from the git hooks/post-receive script.

nikita pushed a commit to branch master
in repository gnurl.

commit 25a25f45ae55aae510a6de1b5bbcfa7547f5db6c
Author: Laramie Leavitt <laramie.leavitt@gmail.com>
AuthorDate: Fri Jul 3 13:10:27 2020 -0700

    http: consolidate nghttp2_session_mem_recv() call paths
    
    Previously there were several locations that called
    nghttp2_session_mem_recv and handled responses slightly differently.
    Those have been converted to call the existing
    h2_process_pending_input() function.
    
    Moved the end-of-session check to h2_process_pending_input() since the
    only place the end-of-session state can change is after nghttp2
    processes additional input frames.
    
    This will likely fix the fuzzing error. While I don't have a root cause
    the out-of-bounds read seems like a use after free, so moving the
    nghttp2_session_check_request_allowed() call to a location with a
    guaranteed nghttp2 session seems reasonable.
    
    Also updated a few nghttp2 callsites to include error messages and added
    a few additional error checks.
    
    Closes #5648
---
 lib/http2.c | 117 +++++++++++++++---------------------------------------------
 1 file changed, 29 insertions(+), 88 deletions(-)

diff --git a/lib/http2.c b/lib/http2.c
index d316da8b6..f045d588f 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -1207,13 +1207,6 @@ void Curl_http2_done(struct Curl_easy *data, bool 
premature)
     }
     http->stream_id = 0;
   }
-
-  if(0 == nghttp2_session_check_request_allowed(httpc->h2)) {
-    /* No more requests are allowed in the current session, so the connection
-       may not be reused. This is set when a GOAWAY frame has been received or
-       when the limit of stream identifiers has been reached. */
-    connclose(data->conn, "http/2: No new requests allowed");
-  }
 }
 
 /*
@@ -1288,7 +1281,7 @@ CURLcode Curl_http2_request_upgrade(struct dynbuf *req,
   binlen = nghttp2_pack_settings_payload(binsettings, H2_BINSETTINGS_LEN,
                                          httpc->local_settings,
                                          httpc->local_settings_num);
-  if(!binlen) {
+  if(binlen <= 0) {
     failf(conn->data, "nghttp2 unexpectedly failed on pack_settings_payload");
     Curl_dyn_free(req);
     return CURLE_FAILED_INIT;
@@ -1371,6 +1364,14 @@ static int h2_process_pending_input(struct connectdata 
*conn,
     return -1;
   }
 
+  if(nghttp2_session_check_request_allowed(httpc->h2) == 0) {
+    /* No more requests are allowed in the current session, so
+       the connection may not be reused. This is set when a
+       GOAWAY frame has been received or when the limit of stream
+       identifiers has been reached. */
+    connclose(conn, "http/2: No new requests allowed");
+  }
+
   if(should_close_session(httpc)) {
     H2BUGF(infof(data,
                  "h2_process_pending_input: nothing to do in this session\n"));
@@ -1383,7 +1384,6 @@ static int h2_process_pending_input(struct connectdata 
*conn,
     }
     return -1;
   }
-
   return 0;
 }
 
@@ -1564,8 +1564,6 @@ static int h2_session_send(struct Curl_easy *data,
 static ssize_t http2_recv(struct connectdata *conn, int sockindex,
                           char *mem, size_t len, CURLcode *err)
 {
-  CURLcode result = CURLE_OK;
-  ssize_t rv;
   ssize_t nread;
   struct http_conn *httpc = &conn->proto.httpc;
   struct Curl_easy *data = conn->data;
@@ -1632,8 +1630,7 @@ static ssize_t http2_recv(struct connectdata *conn, int 
sockindex,
       /* We have paused nghttp2, but we have no pause data (see
          on_data_chunk_recv). */
       httpc->pause_stream_id = 0;
-      if(h2_process_pending_input(conn, httpc, &result) != 0) {
-        *err = result;
+      if(h2_process_pending_input(conn, httpc, err) != 0) {
         return -1;
       }
     }
@@ -1661,8 +1658,7 @@ static ssize_t http2_recv(struct connectdata *conn, int 
sockindex,
          frames, then we have to call it again with 0-length data.
          Without this, on_stream_close callback will not be called,
          and stream could be hanged. */
-      if(h2_process_pending_input(conn, httpc, &result) != 0) {
-        *err = result;
+      if(h2_process_pending_input(conn, httpc, err) != 0) {
         return -1;
       }
     }
@@ -1688,7 +1684,6 @@ static ssize_t http2_recv(struct connectdata *conn, int 
sockindex,
     return -1;
   }
   else {
-    char *inbuf;
     /* remember where to store incoming data for this stream and how big the
        buffer is */
     stream->mem = mem;
@@ -1697,16 +1692,15 @@ static ssize_t http2_recv(struct connectdata *conn, int 
sockindex,
 
     if(httpc->inbuflen == 0) {
       nread = ((Curl_recv *)httpc->recv_underlying)(
-          conn, FIRSTSOCKET, httpc->inbuf, H2_BUFSIZE, &result);
+          conn, FIRSTSOCKET, httpc->inbuf, H2_BUFSIZE, err);
 
       if(nread == -1) {
-        if(result != CURLE_AGAIN)
+        if(*err != CURLE_AGAIN)
           failf(data, "Failed receiving HTTP2 data");
         else if(stream->closed)
           /* received when the stream was already closed! */
           return http2_handle_stream_close(conn, data, stream, err);
 
-        *err = result;
         return -1;
       }
 
@@ -1719,47 +1713,18 @@ static ssize_t http2_recv(struct connectdata *conn, int 
sockindex,
       H2BUGF(infof(data, "nread=%zd\n", nread));
 
       httpc->inbuflen = nread;
-      inbuf = httpc->inbuf;
+
+      DEBUGASSERT(httpc->nread_inbuf == 0);
     }
     else {
       nread = httpc->inbuflen - httpc->nread_inbuf;
-      inbuf = httpc->inbuf + httpc->nread_inbuf;
-
+      (void)nread;  /* silence warning, used in debug */
       H2BUGF(infof(data, "Use data left in connection buffer, nread=%zd\n",
                    nread));
     }
-    rv = nghttp2_session_mem_recv(httpc->h2, (const uint8_t *)inbuf, nread);
 
-    if(nghttp2_is_fatal((int)rv)) {
-      failf(data, "nghttp2_session_mem_recv() returned %zd:%s\n",
-            rv, nghttp2_strerror((int)rv));
-      *err = CURLE_RECV_ERROR;
-      return -1;
-    }
-    H2BUGF(infof(data, "nghttp2_session_mem_recv() returns %zd\n", rv));
-    if(nread == rv) {
-      H2BUGF(infof(data, "All data in connection buffer processed\n"));
-      httpc->inbuflen = 0;
-      httpc->nread_inbuf = 0;
-    }
-    else {
-      httpc->nread_inbuf += rv;
-      H2BUGF(infof(data, "%zu bytes left in connection buffer\n",
-                   httpc->inbuflen - httpc->nread_inbuf));
-    }
-    /* Always send pending frames in nghttp2 session, because
-       nghttp2_session_mem_recv() may queue new frame */
-    rv = h2_session_send(data, httpc->h2);
-    if(rv != 0) {
-      *err = CURLE_SEND_ERROR;
-      return -1;
-    }
-
-    if(should_close_session(httpc)) {
-      H2BUGF(infof(data, "http2_recv: nothing to do in this session\n"));
-      *err = CURLE_HTTP2;
+    if(h2_process_pending_input(conn, httpc, err) != 0)
       return -1;
-    }
   }
   if(stream->memlen) {
     ssize_t retlen = stream->memlen;
@@ -2112,7 +2077,7 @@ static ssize_t http2_send(struct connectdata *conn, int 
sockindex,
   h2_pri_spec(conn->data, &pri_spec);
 
   H2BUGF(infof(conn->data, "http2_send request allowed %d (easy handle %p)\n",
-               nghttp2_session_check_request_allowed(h2), (void *)conn->data));
+         nghttp2_session_check_request_allowed(h2), (void *)conn->data));
 
   switch(conn->data->state.httpreq) {
   case HTTPREQ_POST:
@@ -2138,7 +2103,9 @@ static ssize_t http2_send(struct connectdata *conn, int 
sockindex,
   Curl_safefree(nva);
 
   if(stream_id < 0) {
-    H2BUGF(infof(conn->data, "http2_send() send error\n"));
+    H2BUGF(infof(conn->data,
+                 "http2_send() nghttp2_submit_request error (%s)%d\n",
+                 nghttp2_strerror(stream_id), stream_id));
     *err = CURLE_SEND_ERROR;
     return -1;
   }
@@ -2148,10 +2115,13 @@ static ssize_t http2_send(struct connectdata *conn, int 
sockindex,
   stream->stream_id = stream_id;
 
   /* this does not call h2_session_send() since there can not have been any
-   * priority upodate since the nghttp2_submit_request() call above */
+   * priority update since the nghttp2_submit_request() call above */
   rv = nghttp2_session_send(h2);
-
   if(rv != 0) {
+    H2BUGF(infof(conn->data,
+                 "http2_send() nghttp2_session_send error (%s)%d\n",
+                 nghttp2_strerror(rv), rv));
+
     *err = CURLE_SEND_ERROR;
     return -1;
   }
@@ -2236,7 +2206,6 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
   CURLcode result;
   struct http_conn *httpc = &conn->proto.httpc;
   int rv;
-  ssize_t nproc;
   struct Curl_easy *data = conn->data;
   struct HTTP *stream = conn->data->req.protop;
 
@@ -2310,41 +2279,13 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
 
   if(nread)
     memcpy(httpc->inbuf, mem, nread);
-  httpc->inbuflen = nread;
-
-  nproc = nghttp2_session_mem_recv(httpc->h2, (const uint8_t *)httpc->inbuf,
-                                   httpc->inbuflen);
-
-  if(nghttp2_is_fatal((int)nproc)) {
-    failf(data, "nghttp2_session_mem_recv() failed: %s(%d)",
-          nghttp2_strerror((int)nproc), (int)nproc);
-    return CURLE_HTTP2;
-  }
-
-  H2BUGF(infof(data, "nghttp2_session_mem_recv() returns %zd\n", nproc));
-
-  if((ssize_t)nread == nproc) {
-    httpc->inbuflen = 0;
-    httpc->nread_inbuf = 0;
-  }
-  else {
-    httpc->nread_inbuf += nproc;
-  }
 
-  /* Try to send some frames since we may read SETTINGS already. */
-  rv = h2_session_send(data, httpc->h2);
+  httpc->inbuflen = nread;
 
-  if(rv != 0) {
-    failf(data, "nghttp2_session_send() failed: %s(%d)",
-          nghttp2_strerror(rv), rv);
-    return CURLE_HTTP2;
-  }
+  DEBUGASSERT(httpc->nread_inbuf == 0);
 
-  if(should_close_session(httpc)) {
-    H2BUGF(infof(data,
-                 "nghttp2_session_send(): nothing to do in this session\n"));
+  if(-1 == h2_process_pending_input(conn, httpc, &result))
     return CURLE_HTTP2;
-  }
 
   return CURLE_OK;
 }

-- 
To stop receiving notification emails like this one, please contact
gnunet@gnunet.org.