Merge pull request #2604 from cesanta/pipeline

Fix #2592: do not close pipelined connection prematurely
cesanta · Feb 13, 2024 · daa47e8 · daa47e8
2 parents c8d45c9 + 2419f02
commit daa47e8
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 11 deletions.
diff --git a/mongoose.c b/mongoose.c
@@ -2348,7 +2348,7 @@ int mg_http_get_var(const struct mg_str *buf, const char *name, char *dst,
                     size_t dst_len) {
   int len;
   if (dst != NULL && dst_len > 0) {
-    dst[0] = '\0'; // If destination buffer is valid, always nul-terminate it
+    dst[0] = '\0';  // If destination buffer is valid, always nul-terminate it
   }
   if (dst == NULL || dst_len == 0) {
     len = -2;  // Bad destination
@@ -3158,7 +3158,7 @@ long mg_http_upload(struct mg_connection *c, struct mg_http_message *hm,
     res = -3;
   } else if ((size_t) offset + hm->body.len > max_size) {
     mg_http_reply(c, 400, "", "%s: over max size of %lu", path,
-        (unsigned long) max_size);
+                  (unsigned long) max_size);
     res = -4;
   } else {
     struct mg_fd *fd;
@@ -3216,8 +3216,12 @@ static void http_cb(struct mg_connection *c, int ev, void *ev_data) {
       struct mg_str *te;  // Transfer - encoding header
       bool is_chunked = false;
       if (n < 0) {
-        mg_error(c, "HTTP parse, %lu bytes", c->recv.len);
-        mg_hexdump(c->recv.buf, c->recv.len > 16 ? 16 : c->recv.len);
+        // We don't use mg_error() here, to avoid closing pipelined requests
+        // prematurely, see #2592
+        MG_ERROR(("HTTP parse, %lu bytes", c->recv.len));
+        c->is_draining = 1;
+        mg_hexdump(buf, c->recv.len - ofs > 16 ? 16 : c->recv.len - ofs);
+        c->recv.len = 0;
         return;
       }
       if (n == 0) break;        // Request is not buffered yet

diff --git a/src/http.c b/src/http.c
@@ -128,7 +128,7 @@ int mg_http_get_var(const struct mg_str *buf, const char *name, char *dst,
                     size_t dst_len) {
   int len;
   if (dst != NULL && dst_len > 0) {
-    dst[0] = '\0'; // If destination buffer is valid, always nul-terminate it
+    dst[0] = '\0';  // If destination buffer is valid, always nul-terminate it
   }
   if (dst == NULL || dst_len == 0) {
     len = -2;  // Bad destination
@@ -938,7 +938,7 @@ long mg_http_upload(struct mg_connection *c, struct mg_http_message *hm,
     res = -3;
   } else if ((size_t) offset + hm->body.len > max_size) {
     mg_http_reply(c, 400, "", "%s: over max size of %lu", path,
-        (unsigned long) max_size);
+                  (unsigned long) max_size);
     res = -4;
   } else {
     struct mg_fd *fd;
@@ -996,8 +996,12 @@ static void http_cb(struct mg_connection *c, int ev, void *ev_data) {
       struct mg_str *te;  // Transfer - encoding header
       bool is_chunked = false;
       if (n < 0) {
-        mg_error(c, "HTTP parse, %lu bytes", c->recv.len);
-        mg_hexdump(c->recv.buf, c->recv.len > 16 ? 16 : c->recv.len);
+        // We don't use mg_error() here, to avoid closing pipelined requests
+        // prematurely, see #2592
+        MG_ERROR(("HTTP parse, %lu bytes", c->recv.len));
+        c->is_draining = 1;
+        mg_hexdump(buf, c->recv.len - ofs > 16 ? 16 : c->recv.len - ofs);
+        c->recv.len = 0;
         return;
       }
       if (n == 0) break;        // Request is not buffered yet

diff --git a/test/unit_test.c b/test/unit_test.c
@@ -1376,18 +1376,33 @@ static void f5(struct mg_connection *c, int ev, void *ev_data) {
   }
 }
 
+static void f6(struct mg_connection *c, int ev, void *ev_data) {
+  if (ev == MG_EV_HTTP_MSG) {
+    (*(int *) c->fn_data)++;
+    (void) ev_data;
+  }
+}
+
 static void test_http_pipeline(void) {
   struct mg_mgr mgr;
   const char *url = "http://127.0.0.1:12377";
   struct mg_connection *c;
-  int i, ok = 0;
+  int i, ok = 0, ok2 = 0;
   mg_mgr_init(&mgr);
   mg_http_listen(&mgr, url, f5, (void *) &ok);
-  c = mg_http_connect(&mgr, url, NULL, NULL);
+  c = mg_http_connect(&mgr, url, f6, &ok2);
   mg_printf(c, "POST / HTTP/1.0\nContent-Length: 5\n\n12345GET / HTTP/1.0\n\n");
   for (i = 0; i < 20; i++) mg_mgr_poll(&mgr, 1);
-  // MG_INFO(("-----> [%d]", ok));
   ASSERT(ok == 2);
+  ASSERT(ok2 == 2);
+  // Fire a valid, then invalid request, see #2592. First one should serve
+  ok = ok2 = 0;
+  c = mg_http_connect(&mgr, url, f6, (void *) &ok2);
+  mg_printf(c, "GET / HTTP/1.1\n\nInvalid\n\n");
+  for (i = 0; i < 20; i++) mg_mgr_poll(&mgr, 1);
+  ASSERT(ok == 1);
+  ASSERT(ok2 == 1);
+  //MG_INFO(("-----> [%d] [%d]", ok, ok2));
   mg_mgr_free(&mgr);
   ASSERT(mgr.conns == NULL);
 }