h2c Interop issue - nghttpx & curl #103

LPardue · 2014-11-12T15:22:32Z

We are experiencing an intermittent interop issue between nghttpx (hosted at http://nghttp2.org) and curl 7.39.0 built with nghttp2 0.6.6 when performing a HTTP/2 upgrade to h2c.

The upgrade works always as expected but 1-in-3 times the response is not correctly processed by curl. A packet trace indicates that curl receives a SETTINGS frame after it was expected - the server sends HEADERS and DATA before the SETTINGS. The client then sends a GOAWAY with PROTOCOL_ERROR (SETTINGS expected), curl does not report the returned file(s) and exits

It seems like nghttpx is not following the expected sequence as defined in section 3.5 but I would like to confirm this.

The server connection preface consists of a potentially empty SETTINGS frame (Section 6.5) that MUST be the first frame the server sends in the HTTP/2 connection.

NOTE: we are using a SOCKS proxy to ensure that no intermediary is affecting the upgrade header.

Command line
curl --http2 -v http://nghttp2.org

The text was updated successfully, but these errors were encountered:

tatsuhiro-t · 2014-11-12T17:10:42Z

I found the bug in Curl code to handle HTTP/2.
The bug happens when first incoming SETTINGS (sever sent) is in the same buffer of upgrade response header. Current code does not deal with this situation and SETTINGS frame is discarded.
Therefore GOAWAY (SETTINGS expected) is issued.
Here is a patch to fix this:

From d1a1280c46b5564ddb199ff227a5935fffea9e9f Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Thu, 13 Nov 2014 02:07:24 +0900
Subject: [PATCH] http2: Deal with HTTP/2 data inside response header buffer

---
 lib/http.c  |  8 +++++---
 lib/http2.c | 12 +++++++++++-
 lib/http2.h |  3 ++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/http.c b/lib/http.c
index 2487bac..9083337 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -1746,7 +1746,7 @@ CURLcode Curl_http(struct connectdata *conn, bool *done)
       if(result)
         return result;

-      result = Curl_http2_switched(conn);
+      result = Curl_http2_switched(conn, NULL, 0);
       if(result)
         return result;
       break;
@@ -2993,10 +2993,12 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data,
             infof(data, "Received 101\n");
             k->upgr101 = UPGR101_RECEIVED;

-            /* switch to http2 now */
-            result = Curl_http2_switched(conn);
+            /* switch to http2 now. The bytes after response headers
+               are also processed here, otherwise they are lost. */
+            result = Curl_http2_switched(conn, k->str, *nread);
             if(result)
               return result;
+            *nread = 0;
           }
           break;
         default:
diff --git a/lib/http2.c b/lib/http2.c
index f86d3eb..8cb86a0 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -988,7 +988,8 @@ CURLcode Curl_http2_setup(struct connectdata *conn)
   return 0;
 }

-CURLcode Curl_http2_switched(struct connectdata *conn)
+CURLcode Curl_http2_switched(struct connectdata *conn,
+                             const char *mem, ssize_t nread)
 {
   CURLcode rc;
   struct http_conn *httpc = &conn->proto.httpc;
@@ -1036,6 +1037,15 @@ CURLcode Curl_http2_switched(struct connectdata *conn)
       return CURLE_HTTP2;
     }
   }
+
+  rv = nghttp2_session_mem_recv(httpc->h2, (const uint8_t*)mem, nread);
+
+  if(rv != nread) {
+    failf(data, "nghttp2_session_mem_recv() failed: %s(%d)",
+          nghttp2_strerror(rv), rv);
+    return CURLE_HTTP2;
+  }
+
   return CURLE_OK;
 }

diff --git a/lib/http2.h b/lib/http2.h
index 66aa6fd..497c451 100644
--- a/lib/http2.h
+++ b/lib/http2.h
@@ -37,7 +37,8 @@ CURLcode Curl_http2_send_request(struct connectdata *conn);
 CURLcode Curl_http2_request_upgrade(Curl_send_buffer *req,
                                     struct connectdata *conn);
 CURLcode Curl_http2_setup(struct connectdata *conn);
-CURLcode Curl_http2_switched(struct connectdata *conn);
+CURLcode Curl_http2_switched(struct connectdata *conn,
+                             const char *data, ssize_t nread);
 #else /* USE_NGHTTP2 */
 #define Curl_http2_init(x) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_send_request(x) CURLE_UNSUPPORTED_PROTOCOL
-- 
2.1.1

LPardue · 2014-11-12T18:17:31Z

The bug happens when first incoming SETTINGS (sever sent) is in the same buffer of upgrade response header.

To clarify, are you saying the SETTINGS frame is appended to the response HTTP/1.1 101 Switching Protocols?

Looking at a packet trace, I can see the this happening in 0x80

0000  08 00 27 19 23 0e 52 54  00 12 35 02 08 00 45 00   ..'.#.RT ..5...E.
0010  00 87 2b de 00 00 40 06  16 5c ac 1d 80 0b 0a 00   ..+...@. .\......
0020  02 0f 04 3d da 4f 0e aa  0a 0a 45 6a 2f 21 50 18   ...=.O.. ..Ej/!P.
0030  ff ff 28 16 00 00 48 54  54 50 2f 31 2e 31 20 31   ..(...HT TP/1.1 1
0040  30 31 20 53 77 69 74 63  68 69 6e 67 20 50 72 6f   01 Switc hing Pro
0050  74 6f 63 6f 6c 73 0d 0a  43 6f 6e 6e 65 63 74 69   tocols.. Connecti
0060  6f 6e 3a 20 55 70 67 72  61 64 65 0d 0a 55 70 67   on: Upgr ade..Upg
0070  72 61 64 65 3a 20 68 32  63 2d 31 34 0d 0a 0d 0a   rade: h2 c-14....
0080  00 00 0c 04 00 00 00 00  00 00 03 00 00 00 64 00   ........ ......d.
0090  04 00 00 ff ff                                     .....

Thank you for the patch, tomorrow, we will test and confirm if it fixes the issue we have seen.

tatsuhiro-t · 2014-11-12T23:10:06Z

Yes, SETTINGS is appended after upgrade response header. This is very common, so it is strange we did not spot it before. Maybe we just focused to TLS too much.
If this patch fixes the issue in your environment, I will send it to libcurl mailing list.

bagder · 2014-11-13T08:49:10Z

Please do Tatsuhiro, this is a curl flaw I've seen in my local branch working on a http2 test suite for curl but I've not gotten as far as actually fixing it (nor merging my test suite work into the master branch yet).

tatsuhiro-t · 2014-11-13T12:18:30Z

OK, I'll post patch to libcurl ML soon.

LPardue · 2014-11-13T14:24:31Z

We can confirm that Tatsuhiro's patch fixes the reported issue.

Credit to Sam Hurst for helping in identifying the issue and testing the patch on our side.

tatsuhiro-t · 2014-11-13T14:41:19Z

Thank you. Since I submitted the patch to libcurl ML and this is libcurl issue, this issue is now closed.

bagder · 2014-11-13T14:41:39Z

Thanks everyone, fix has been merged in curl now.

tatsuhiro-t · 2014-11-13T14:42:29Z

Great!

tatsuhiro-t closed this as completed Nov 13, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

h2c Interop issue - nghttpx & curl #103

h2c Interop issue - nghttpx & curl #103

LPardue commented Nov 12, 2014

tatsuhiro-t commented Nov 12, 2014

LPardue commented Nov 12, 2014

tatsuhiro-t commented Nov 12, 2014

bagder commented Nov 13, 2014

tatsuhiro-t commented Nov 13, 2014

LPardue commented Nov 13, 2014

tatsuhiro-t commented Nov 13, 2014

bagder commented Nov 13, 2014

tatsuhiro-t commented Nov 13, 2014

h2c Interop issue - nghttpx & curl #103

h2c Interop issue - nghttpx & curl #103

Comments

LPardue commented Nov 12, 2014

tatsuhiro-t commented Nov 12, 2014

LPardue commented Nov 12, 2014

tatsuhiro-t commented Nov 12, 2014

bagder commented Nov 13, 2014

tatsuhiro-t commented Nov 13, 2014

LPardue commented Nov 13, 2014

tatsuhiro-t commented Nov 13, 2014

bagder commented Nov 13, 2014

tatsuhiro-t commented Nov 13, 2014