Fix off-by-one in handling upgrade bodies.

- When handling upgraded bodies, http_parser_execute() used to return one fewer bytes parsed than expected. This caused the final LF to be interpreted by the caller as part of the body. - Add a bunch of upgrade body unit tests.
14 years ago · d4ca280af5
parent f684abdcc5
commit d4ca280af5
2 changed files with 112 additions and 37 deletions
--- a/http_parser.c
+++ b/http_parser.c
@ -1439,7 +1439,7 @@ size_t http_parser_execute (http_parser *parser,
        /* Exit, the rest of the connect is in a different protocol. */
        if (parser->upgrade) {
          CALLBACK2(message_complete);
-          return (p - data);
+          return (p - data) + 1;
        }

        if (parser->flags & F_SKIPBODY) {
--- a/test.c
+++ b/test.c
@ -55,7 +55,7 @@ struct message {
  char headers [MAX_HEADERS][2][MAX_ELEMENT_SIZE];
  int should_keep_alive;

-  int upgrade;
+  const char *upgrade; // upgraded body

  unsigned short http_major;
  unsigned short http_minor;
@ -473,6 +473,7 @@ const struct message requests[] =
         "Sec-WebSocket-Key1: 4 @1  46546xW%0l 1 5\r\n"
         "Origin: http://example.com\r\n"
         "\r\n"
+         "Hot diggity dogg"
  ,.should_keep_alive= TRUE
  ,.message_complete_on_eof= FALSE
  ,.http_major= 1
@ -483,7 +484,7 @@ const struct message requests[] =
  ,.request_path= "/demo"
  ,.request_url= "/demo"
  ,.num_headers= 7
-  ,.upgrade=1
+  ,.upgrade="Hot diggity dogg"
  ,.headers= { { "Host", "example.com" }
             , { "Connection", "Upgrade" }
             , { "Sec-WebSocket-Key2", "12998 5 Y3 1  .P00" }
@ -502,6 +503,8 @@ const struct message requests[] =
         "User-agent: Mozilla/1.1N\r\n"
         "Proxy-authorization: basic aGVsbG86d29ybGQ=\r\n"
         "\r\n"
+         "some data\r\n"
+         "and yet even more data"
  ,.should_keep_alive= FALSE
  ,.message_complete_on_eof= FALSE
  ,.http_major= 1
@ -512,7 +515,7 @@ const struct message requests[] =
  ,.request_path= ""
  ,.request_url= "0-home0.netscape.com:443"
  ,.num_headers= 2
-  ,.upgrade=1
+  ,.upgrade="some data\r\nand yet even more data"
  ,.headers= { { "User-agent", "Mozilla/1.1N" }
             , { "Proxy-authorization", "basic aGVsbG86d29ybGQ=" }
             }
@ -707,7 +710,7 @@ const struct message requests[] =
  ,.request_path= ""
  ,.request_url= "home_0.netscape.com:443"
  ,.num_headers= 2
-  ,.upgrade=1
+  ,.upgrade=""
  ,.headers= { { "User-agent", "Mozilla/1.1N" }
             , { "Proxy-authorization", "basic aGVsbG86d29ybGQ=" }
             }
@ -735,7 +738,6 @@ const struct message requests[] =
  ,.request_path= "/file.txt"
  ,.request_url= "/file.txt"
  ,.num_headers= 4
-  ,.upgrade=0
  ,.headers= { { "Host", "www.example.com" }
             , { "Content-Type", "application/example" }
             , { "If-Match", "\"e0023aa4e\"" }
@ -1313,7 +1315,13 @@ check_str_eq (const struct message *m,
              const char *prop,
              const char *expected,
              const char *found) {
-  if (0 != strcmp(expected, found)) {
+  if ((expected == NULL) != (found == NULL)) {
+    printf("\n*** Error: %s in '%s' ***\n\n", prop, m->name);
+    printf("expected %s\n", (expected == NULL) ? "NULL" : expected);
+    printf("   found %s\n", (found == NULL) ? "NULL" : found);
+    return 0;
+  }
+  if (expected != NULL && 0 != strcmp(expected, found)) {
    printf("\n*** Error: %s in '%s' ***\n\n", prop, m->name);
    printf("expected '%s'\n", expected);
    printf("   found '%s'\n", found);
@ -1386,9 +1394,75 @@ message_eq (int index, const struct message *expected)
    if (!r) return 0;
  }

+  MESSAGE_CHECK_STR_EQ(expected, m, upgrade);
+
  return 1;
 }

+/* Given a sequence of varargs messages, return the number of them that the
+ * parser should successfully parse, taking into account that upgraded
+ * messages prevent all subsequent messages from being parsed.
+ */
+size_t
+count_parsed_messages(const size_t nmsgs, ...) {
+  size_t i;
+  va_list ap;
+
+  va_start(ap, nmsgs);
+
+  for (i = 0; i < nmsgs; i++) {
+    struct message *m = va_arg(ap, struct message *);
+
+    if (m->upgrade) {
+      va_end(ap);
+      return i + 1;
+    }
+  }
+
+  va_end(ap);
+  return nmsgs;
+}
+
+/* Given a sequence of bytes and the number of these that we were able to
+ * parse, verify that upgrade bodies are correct.
+ */
+void
+upgrade_message_fix(char *body, const size_t nread, const size_t nmsgs, ...) {
+  va_list ap;
+  size_t i;
+  size_t off = 0;
+ 
+  va_start(ap, nmsgs);
+
+  for (i = 0; i < nmsgs; i++) {
+    struct message *m = va_arg(ap, struct message *);
+
+    off += strlen(m->raw);
+
+    if (m->upgrade) {
+      off -= strlen(m->upgrade);
+
+      /* Check the portion of the response after its specified upgrade */
+      if (!check_str_eq(m, "upgrade", body + off, body + nread)) {
+        exit(1);
+      }
+
+      /* Fix up the response so that message_eq() will verify the beginning
+       * of the upgrade */
+      *(body + nread + strlen(m->upgrade)) = '\0';
+      messages[num_messages -1 ].upgrade = body + nread;
+
+      va_end(ap);
+      return;
+    }
+  }
+
+  va_end(ap);
+  printf("\n\n*** Error: expected a message with upgrade ***\n");
+
+  exit(1);
+}
+
 static void
 print_error (const char *raw, size_t error_location)
 {
@ -1447,7 +1521,10 @@ test_message (const struct message *message)
    if (msg1len) {
      read = parse(msg1, msg1len);

-      if (message->upgrade && parser->upgrade) goto test;
+      if (message->upgrade && parser->upgrade) {
+        messages[num_messages - 1].upgrade = msg1 + read;
+        goto test;
+      }

      if (read != msg1len) {
        print_error(msg1, read);
@ -1458,7 +1535,10 @@ test_message (const struct message *message)

    read = parse(msg2, msg2len);

-    if (message->upgrade && parser->upgrade) goto test;
+    if (message->upgrade && parser->upgrade) {
+      messages[num_messages - 1].upgrade = msg2 + read;
+      goto test;
+    }

    if (read != msg2len) {
      print_error(msg2, read);
@ -1467,8 +1547,6 @@ test_message (const struct message *message)

    read = parse(NULL, 0);

-    if (message->upgrade && parser->upgrade) goto test;
-
    if (read != 0) {
      print_error(message->raw, read);
      exit(1);
@ -1603,12 +1681,7 @@ test_no_overflow_long_body (int req, size_t length)
 void
 test_multiple3 (const struct message *r1, const struct message *r2, const struct message *r3)
 {
-  int message_count = 1;
-  if (!r1->upgrade) {
-    message_count++;
-    if (!r2->upgrade) message_count++;
-  }
-  int has_upgrade = (message_count < 3 || r3->upgrade);
+  int message_count = count_parsed_messages(3, r1, r2, r3);

  char total[ strlen(r1->raw)
            + strlen(r2->raw)
@ -1627,7 +1700,10 @@ test_multiple3 (const struct message *r1, const struct message *r2, const struct

  read = parse(total, strlen(total));

-  if (has_upgrade && parser->upgrade) goto test;
+  if (parser->upgrade) {
+    upgrade_message_fix(total, read, 3, r1, r2, r3);
+    goto test;
+  }

  if (read != strlen(total)) {
    print_error(total, read);
@ -1636,8 +1712,6 @@ test_multiple3 (const struct message *r1, const struct message *r2, const struct

  read = parse(NULL, 0);

-  if (has_upgrade && parser->upgrade) goto test;
-
  if (read != 0) {
    print_error(total, read);
    exit(1);
@ -1651,12 +1725,8 @@ test:
  }

  if (!message_eq(0, r1)) exit(1);
-  if (message_count > 1) {
-    if (!message_eq(1, r2)) exit(1);
-    if (message_count > 2) {
-      if (!message_eq(2, r3)) exit(1);
-    }
-  }
+  if (message_count > 1 && !message_eq(1, r2)) exit(1);
+  if (message_count > 2 && !message_eq(2, r3)) exit(1);

  parser_free();
 }
@ -1685,6 +1755,7 @@ test_scan (const struct message *r1, const struct message *r2, const struct mess
  int ops = 0 ;

  size_t buf1_len, buf2_len, buf3_len;
+  int message_count = count_parsed_messages(3, r1, r2, r3);

  int i,j,type_both;
  for (type_both = 0; type_both < 2; type_both ++ ) {
@ -1713,27 +1784,27 @@ test_scan (const struct message *r1, const struct message *r2, const struct mess

        read = parse(buf1, buf1_len);

-        if (r3->upgrade && parser->upgrade) goto test;
+        if (parser->upgrade) goto test;

        if (read != buf1_len) {
          print_error(buf1, read);
          goto error;
        }

-        read = parse(buf2, buf2_len);
+        read += parse(buf2, buf2_len);

-        if (r3->upgrade && parser->upgrade) goto test;
+        if (parser->upgrade) goto test;

-        if (read != buf2_len) {
+        if (read != buf1_len + buf2_len) {
          print_error(buf2, read);
          goto error;
        }

-        read = parse(buf3, buf3_len);
+        read += parse(buf3, buf3_len);

-        if (r3->upgrade && parser->upgrade) goto test;
+        if (parser->upgrade) goto test;

-        if (read != buf3_len) {
+        if (read != buf1_len + buf2_len + buf3_len) {
          print_error(buf3, read);
          goto error;
        }
@ -1741,9 +1812,13 @@ test_scan (const struct message *r1, const struct message *r2, const struct mess
        parse(NULL, 0);

 test:
+        if (parser->upgrade) {
+          upgrade_message_fix(total, read, 3, r1, r2, r3);
+        }

-        if (3 != num_messages) {
-          fprintf(stderr, "\n\nParser didn't see 3 messages only %d\n", num_messages);
+        if (message_count != num_messages) {
+          fprintf(stderr, "\n\nParser didn't see %d messages only %d\n",
+            message_count, num_messages);
          goto error;
        }

@ -1752,12 +1827,12 @@ test:
          goto error;
        }

-        if (!message_eq(1, r2)) {
+        if (message_count > 1 && !message_eq(1, r2)) {
          fprintf(stderr, "\n\nError matching messages[1] in test_scan.\n");
          goto error;
        }

-        if (!message_eq(2, r3)) {
+        if (message_count > 2 && !message_eq(2, r3)) {
          fprintf(stderr, "\n\nError matching messages[2] in test_scan.\n");
          goto error;
        }