http: handle trailer headers in all chunked responses

HTTP allows that a server sends trailing headers after all the chunks
have been sent WITHOUT signalling their presence in the first response
headers. The "Trailer:" header is only a SHOULD there and as we need to
handle the situation even without that header I made libcurl ignore
Trailer: completely.

Test case 1116 was added to verify this and to make sure we handle more
than one trailer header properly.

Reported by: Patrick McManus
Bug: http://curl.haxx.se/bug/view.cgi?id=3052450

6 files changed, 147 insertions, 104 deletions

diff --git a/lib/http.c b/lib/http.c
index 2d0e78af3..3d6f977ea 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -3647,20 +3647,6 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data,
       /* init our chunky engine */
       Curl_httpchunk_init(conn);
     }
-
-    else if(checkprefix("Trailer:", k->p) ||
-            checkprefix("Trailers:", k->p)) {
-      /*
-       * This test helps Curl_httpchunk_read() to determine to look
-       * for well formed trailers after the zero chunksize record. In
-       * this case a CRLF is required after the zero chunksize record
-       * when no trailers are sent, or after the last trailer record.
-       *
-       * It seems both Trailer: and Trailers: occur in the wild.
-       */
-      k->trailerhdrpresent = TRUE;
-    }
-
     else if(checkprefix("Content-Encoding:", k->p) &&
             data->set.str[STRING_ENCODING]) {
       /*
diff --git a/lib/http_chunks.c b/lib/http_chunks.c
index a66f87210..0d41979af 100644
--- a/lib/http_chunks.c
+++ b/lib/http_chunks.c
@@ -184,22 +184,8 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
       if(*datap == 0x0a) {
         /* we're now expecting data to come, unless size was zero! */
         if(0 == ch->datasize) {
-          if(k->trailerhdrpresent!=TRUE) {
-            /* No Trailer: header found - revert to original Curl processing */
-            ch->state = CHUNK_STOPCR;
-
-            /* We need to increment the datap here since we bypass the
-               increment below with the immediate break */
-            length--;
-            datap++;
-
-            /* This is the final byte, continue to read the final CRLF */
-            break;
-          }
-          else {
-            ch->state = CHUNK_TRAILER; /* attempt to read trailers */
-            conn->trlPos=0;
-          }
+          ch->state = CHUNK_TRAILER; /* now check for trailers */
+          conn->trlPos=0;
         }
         else {
           ch->state = CHUNK_DATA;
@@ -280,9 +266,9 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
         datap++;
         length--;
       }
-      else {
+      else
         return CHUNKE_BAD_CHUNK;
-      }
+
       break;
 
     case CHUNK_POSTLF:
@@ -295,80 +281,32 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
         datap++;
         length--;
       }
-      else {
+      else
         return CHUNKE_BAD_CHUNK;
-      }
 
       break;
 
     case CHUNK_TRAILER:
-      /* conn->trailer is assumed to be freed in url.c on a
-         connection basis */
-      if(conn->trlPos >= conn->trlMax) {
-        /* in this logic we always allocate one byte more than trlMax
-           contains, just because CHUNK_TRAILER_POSTCR will append two bytes
-           so we need to make sure we have room for an extra byte */
-        char *ptr;
-        if(conn->trlMax) {
-          conn->trlMax *= 2;
-          ptr = realloc(conn->trailer, conn->trlMax + 1);
-        }
-        else {
-          conn->trlMax=128;
-          ptr = malloc(conn->trlMax + 1);
-        }
-        if(!ptr)
-          return CHUNKE_OUT_OF_MEMORY;
-        conn->trailer = ptr;
-      }
-      conn->trailer[conn->trlPos++]=*datap;
-
-      if(*datap == 0x0d)
-        ch->state = CHUNK_TRAILER_CR;
-      else {
-        datap++;
-        length--;
-      }
-      break;
-
-    case CHUNK_TRAILER_CR:
       if(*datap == 0x0d) {
-        ch->state = CHUNK_TRAILER_POSTCR;
-        datap++;
-        length--;
-      }
-      else
-        return CHUNKE_BAD_CHUNK;
-      break;
-
-    case CHUNK_TRAILER_POSTCR:
-      if(*datap == 0x0a) {
-        conn->trailer[conn->trlPos++]=0x0a;
-        conn->trailer[conn->trlPos]=0;
-        if(conn->trlPos==2) {
-          ch->state = CHUNK_STOP;
-          length--;
+        /* this is the end of a trailer, but if the trailer was zero bytes
+           there was no trailer and we move on */
 
-          /*
-           * Note that this case skips over the final STOP states since we've
-           * already read the final CRLF and need to return
-           */
+        if(conn->trlPos) {
+          /* we allocate trailer with 3 bytes extra room to fit this */
+          conn->trailer[conn->trlPos++]=0x0d;
+          conn->trailer[conn->trlPos++]=0x0a;
+          conn->trailer[conn->trlPos]=0;
 
-          ch->dataleft = length;
-
-          return CHUNKE_STOP; /* return stop */
-        }
-        else {
 #ifdef CURL_DOES_CONVERSIONS
           /* Convert to host encoding before calling Curl_client_write */
           result = Curl_convert_from_network(conn->data,
                                              conn->trailer,
                                              conn->trlPos);
-          if(result != CURLE_OK) {
+          if(result != CURLE_OK)
             /* Curl_convert_from_network calls failf if unsuccessful */
             /* Treat it as a bad chunk */
-            return(CHUNKE_BAD_CHUNK);
-          }
+            return CHUNKE_BAD_CHUNK;
+
 #endif /* CURL_DOES_CONVERSIONS */
           if(!data->set.http_te_skip) {
             result = Curl_client_write(conn, CLIENTWRITE_HEADER,
@@ -376,9 +314,44 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
             if(result)
               return CHUNKE_WRITE_ERROR;
           }
+          conn->trlPos=0;
+          ch->state = CHUNK_TRAILER_CR;
         }
-        ch->state = CHUNK_TRAILER;
-        conn->trlPos=0;
+        else {
+          /* no trailer, we're on the final CRLF pair */
+          ch->state = CHUNK_TRAILER_POSTCR;
+          break; /* don't advance the pointer */
+        }
+      }
+      else {
+        /* conn->trailer is assumed to be freed in url.c on a
+           connection basis */
+        if(conn->trlPos >= conn->trlMax) {
+          /* we always allocate three extra bytes, just because when the full
+             header has been received we append CRLF\0 */
+          char *ptr;
+          if(conn->trlMax) {
+            conn->trlMax *= 2;
+            ptr = realloc(conn->trailer, conn->trlMax + 3);
+          }
+          else {
+            conn->trlMax=128;
+            ptr = malloc(conn->trlMax + 3);
+          }
+          if(!ptr)
+            return CHUNKE_OUT_OF_MEMORY;
+          conn->trailer = ptr;
+        }
+        fprintf(stderr, "MOO: %c\n", *datap);
+        conn->trailer[conn->trlPos++]=*datap;
+      }
+      datap++;
+      length--;
+      break;
+
+    case CHUNK_TRAILER_CR:
+      if(*datap == 0x0a) {
+        ch->state = CHUNK_TRAILER_POSTCR;
         datap++;
         length--;
       }
@@ -386,6 +359,20 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
         return CHUNKE_BAD_CHUNK;
       break;
 
+    case CHUNK_TRAILER_POSTCR:
+      /* We enter this state when a CR should arrive so we expect to
+         have to first pass a CR before we wait for LF */
+      if(*datap != 0x0d) {
+        /* not a CR then it must be another header in the trailer */
+        ch->state = CHUNK_TRAILER;
+        break;
+      }
+      datap++;
+      length--;
+      /* now wait for the final LF */
+      ch->state = CHUNK_STOP;
+      break;
+
     case CHUNK_STOPCR:
       /* Read the final CRLF that ends all chunk bodies */
 
@@ -394,9 +381,8 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
         datap++;
         length--;
       }
-      else {
+      else
         return CHUNKE_BAD_CHUNK;
-      }
       break;
 
     case CHUNK_STOP:
@@ -409,9 +395,8 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn,
         ch->dataleft = length;
         return CHUNKE_STOP; /* return stop */
       }
-      else {
+      else
         return CHUNKE_BAD_CHUNK;
-      }
 
     default:
       return CHUNKE_STATE_ERROR;
diff --git a/lib/url.c b/lib/url.c
index ac621f220..3ae797532 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -5300,10 +5300,8 @@ static CURLcode do_init(struct connectdata *conn)
 static void do_complete(struct connectdata *conn)
 {
   conn->data->req.chunk=FALSE;
-  conn->data->req.trailerhdrpresent=FALSE;
-
   conn->data->req.maxfd = (conn->sockfd>conn->writesockfd?
-                               conn->sockfd:conn->writesockfd)+1;
+                           conn->sockfd:conn->writesockfd)+1;
 }
 
 CURLcode Curl_do(struct connectdata **connp, bool *done)
diff --git a/lib/urldata.h b/lib/urldata.h
index de9acf8bf..940cb3551 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -588,9 +588,6 @@ struct SingleRequest {
   bool forbidchunk;   /* used only to explicitly forbid chunk-upload for
                          specific upload buffers. See readmoredata() in
                          http.c for details. */
-  bool trailerhdrpresent; /* Set when Trailer: header found in HTTP response.
-                             Required to determine whether to look for trailers
-                             in case of Transfer-Encoding: chunking */
 };
 
 /*
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 2714b3dd2..a31b35665 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -67,7 +67,7 @@ EXTRA_DIST = test1 test108 test117 test127 test20 test27 test34 test46	   \
  test312 test1105 test565 test800 test1106 test801 test566 test802 test803 \
  test1107 test1108 test1109 test1110 test1111 test1112 test129 test567     \
  test568 test569 test570 test571 test572 test804 test805 test806 test807 \
- test573 test313 test1115 test578 test579
+ test573 test313 test1115 test578 test579 test1116
 
 filecheck:
 	@mkdir test-place; \
diff --git a/tests/data/test1116 b/tests/data/test1116
new file mode 100644
index 000000000..a9af3e61b
--- /dev/null
+++ b/tests/data/test1116
@@ -0,0 +1,77 @@
+<testcase>
+<info>
+<keywords>
+HTTP
+HTTP GET
+chunked Transfer-Encoding
+</keywords>
+</info>
+#
+# Server-side
+<reply>
+<data>
+HTTP/1.1 200 funky chunky!

+Server: fakeit/0.9 fakeitbad/1.0

+Transfer-Encoding: chunked

+Connection: mooo

+

+40

+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

+30

+bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

+21;heresatest=moooo

+cccccccccccccccccccccccccccccccc
+

+0

+chunky-trailer: header data

+another-header: yes

+

+</data>
+<datacheck>
+HTTP/1.1 200 funky chunky!

+Server: fakeit/0.9 fakeitbad/1.0

+Transfer-Encoding: chunked

+Connection: mooo

+

+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccccccccccccc
+</datacheck>
+</reply>
+
+#
+# Client-side
+<client>
+<server>
+http
+</server>
+ <name>
+HTTP GET with chunked trailer without Trailer:
+ </name>
+ <command>
+http://%HOSTIP:%HTTPPORT/1116 -D log/heads1116
+</command>
+</client>
+
+#
+# Verify data after the test has been "shot"
+<verify>
+<strip>
+^User-Agent:.*
+</strip>
+<protocol>
+GET /1116 HTTP/1.1

+Host: %HOSTIP:%HTTPPORT

+Accept: */*

+

+</protocol>
+<file name="log/heads1116">
+HTTP/1.1 200 funky chunky!

+Server: fakeit/0.9 fakeitbad/1.0

+Transfer-Encoding: chunked

+Connection: mooo

+

+chunky-trailer: header data

+another-header: yes

+</file>
+</verify>
+
+</testcase>