Fix remainingStreamingBytes timing: set before requestHandler, decrement before dataHandler

src/HttpParser.h

@@ -533,15 +533,6 @@ struct HttpParser {
             const char *querySeparatorPtr = (const char *) memchr(req->headers->value.data(), '?', req->headers->value.length());
             req->querySeparator = (unsigned int) ((querySeparatorPtr ? querySeparatorPtr : req->headers->value.data() + req->headers->value.length()) - req->headers->value.data());
 
-            /* If returned socket is not what we put in we need
-             * to break here as we either have upgraded to
-             * WebSockets or otherwise closed the socket. */
-            void *returnedUser = requestHandler(user, req);
-            if (returnedUser != user) {
-                /* We are upgraded to WebSocket or otherwise broken */
-                return {consumedTotal, returnedUser};
-            }
-
             /* The rules at play here according to RFC 9112 for requests are essentially:
              * If both content-length and transfer-encoding then invalid message; must break.
              * If has transfer-encoding then must be chunked regardless of value.
@@ -566,6 +557,28 @@ struct HttpParser {
                  * This could be made stricter but makes no difference either way, unless forwarding the identical message as a proxy. */
 
                 remainingStreamingBytes = STATE_IS_CHUNKED;
+            } else if (contentLengthString.length()) {
+                remainingStreamingBytes = toUnsignedInteger(contentLengthString);
+                if (remainingStreamingBytes == UINT64_MAX) {
+                    /* Parser error */
+                    return {HTTP_ERROR_400_BAD_REQUEST, FULLPTR};
+                }
+            } else {
+                /* No body (e.g. GET requests); set to 0 to match this assumption */
+                remainingStreamingBytes = 0;
+            }
+
+            /* If returned socket is not what we put in we need
+             * to break here as we either have upgraded to
+             * WebSockets or otherwise closed the socket. */
+            void *returnedUser = requestHandler(user, req);
+            if (returnedUser != user) {
+                /* We are upgraded to WebSocket or otherwise broken */
+                return {consumedTotal, returnedUser};
+            }
+
+            /* Consume body data */
+            if (transferEncodingString.length()) {
                 /* If consume minimally, we do not want to consume anything but we want to mark this as being chunked */
                 if (!CONSUME_MINIMALLY) {
                     /* Go ahead and parse it (todo: better heuristics for emitting FIN to the app level) */
@@ -582,16 +595,11 @@ struct HttpParser {
                     consumedTotal += consumed;
                 }
             } else if (contentLengthString.length()) {
-                remainingStreamingBytes = toUnsignedInteger(contentLengthString);
-                if (remainingStreamingBytes == UINT64_MAX) {
-                    /* Parser error */
-                    return {HTTP_ERROR_400_BAD_REQUEST, FULLPTR};
-                }
-
                 if (!CONSUME_MINIMALLY) {
                     unsigned int emittable = (unsigned int) std::min<uint64_t>(remainingStreamingBytes, length);
-                    dataHandler(user, std::string_view(data, emittable), emittable == remainingStreamingBytes);
+                    bool fin = (emittable == remainingStreamingBytes);
                     remainingStreamingBytes -= emittable;
+                    dataHandler(user, std::string_view(data, emittable), fin);
 
                     data += emittable;
                     length -= emittable;
@@ -646,16 +654,17 @@ struct HttpParser {
                 // this is exactly the same as below!
                 // todo: refactor this
                 if (remainingStreamingBytes >= length) {
-                    void *returnedUser = dataHandler(user, std::string_view(data, length), remainingStreamingBytes == length);
+                    bool fin = (remainingStreamingBytes == length);
                     remainingStreamingBytes -= length;
+                    void *returnedUser = dataHandler(user, std::string_view(data, length), fin);
                     return {0, returnedUser};
                 } else {
-                    void *returnedUser = dataHandler(user, std::string_view(data, remainingStreamingBytes), true);
-
-                    data += (unsigned int) remainingStreamingBytes;
-                    length -= (unsigned int) remainingStreamingBytes;
-
+                    unsigned int toEmit = (unsigned int) remainingStreamingBytes;
                     remainingStreamingBytes = 0;
+                    void *returnedUser = dataHandler(user, std::string_view(data, toEmit), true);
+
+                    data += toEmit;
+                    length -= toEmit;
 
                     if (returnedUser != user) {
                         return {0, returnedUser};
@@ -702,16 +711,17 @@ struct HttpParser {
                     } else {
                         // this is exactly the same as above!
                         if (remainingStreamingBytes >= (unsigned int) length) {
-                            void *returnedUser = dataHandler(user, std::string_view(data, length), remainingStreamingBytes == (unsigned int) length);
+                            bool fin = (remainingStreamingBytes == (unsigned int) length);
                             remainingStreamingBytes -= length;
+                            void *returnedUser = dataHandler(user, std::string_view(data, length), fin);
                             return {0, returnedUser};
                         } else {
-                            void *returnedUser = dataHandler(user, std::string_view(data, remainingStreamingBytes), true);
-
-                            data += (unsigned int) remainingStreamingBytes;
-                            length -= (unsigned int) remainingStreamingBytes;
-
+                            unsigned int toEmit = (unsigned int) remainingStreamingBytes;
                             remainingStreamingBytes = 0;
+                            void *returnedUser = dataHandler(user, std::string_view(data, toEmit), true);
+
+                            data += toEmit;
+                            length -= toEmit;
 
                             if (returnedUser != user) {
                                 return {0, returnedUser};

tests/.gitignore

@@ -0,0 +1,7 @@
+BloomFilter
+ChunkedEncoding
+ExtensionsNegotiator
+HttpParser
+HttpRouter
+Query
+TopicTree

tests/HttpParser.cpp

@@ -12,7 +12,7 @@ int main() {
 
     uWS::HttpParser httpParser;
 
-    auto [err, returnedUser] = httpParser.consumePostPadded((char *) data, size, user, reserved, [reserved](void *s, uWS::HttpRequest *httpRequest) -> void * {
+    auto [err, returnedUser] = httpParser.consumePostPadded((char *) data, size, user, reserved, [reserved, &httpParser](void *s, uWS::HttpRequest *httpRequest) -> void * {
 
         std::cout << httpRequest->getMethod() << std::endl;
 
@@ -23,6 +23,9 @@ int main() {
         /* Since we did proper whitespace trimming this thing is there, but empty */
         assert(httpRequest->getHeader("utf8").data());
 
+        /* maxRemainingBodyLength() must be 0 for GET requests (no body) when called inside requestHandler */
+        assert(httpParser.maxRemainingBodyLength() == 0);
+
         /* Return ok */
         return s;
 
@@ -35,4 +38,30 @@ int main() {
 
     std::cout << "HTTP DONE" << std::endl;
 
+    /* Test that maxRemainingBodyLength() is set correctly before requestHandler for a POST with content-length */
+    {
+        /* POST / HTTP/1.1\r\nHost: localhost\r\nContent-Length: 5\r\n\r\nhello + 8 bytes padding */
+        const char postData[] = "POST / HTTP/1.1\r\nHost: localhost\r\nContent-Length: 5\r\n\r\nhelloEEEEEEEE";
+        int postSize = (int)(sizeof(postData) - 1 - 8);
+        uWS::HttpParser postParser;
+        uint64_t bodyLengthInHandler = UINT64_MAX;
+        uint64_t bodyLengthInDataHandler = UINT64_MAX;
+
+        postParser.consumePostPadded((char *) postData, postSize, user, reserved, [&postParser, &bodyLengthInHandler](void *s, uWS::HttpRequest *httpRequest) -> void * {
+            bodyLengthInHandler = postParser.maxRemainingBodyLength();
+            return s;
+        }, [&postParser, &bodyLengthInDataHandler](void *user, std::string_view data, bool fin) -> void * {
+            /* maxRemainingBodyLength() must be decremented before dataHandler is called */
+            bodyLengthInDataHandler = postParser.maxRemainingBodyLength();
+            return user;
+        });
+
+        /* maxRemainingBodyLength() must return content-length (5) when called inside requestHandler */
+        assert(bodyLengthInHandler == 5);
+        std::cout << "POST content-length in requestHandler test PASS" << std::endl;
+        /* maxRemainingBodyLength() must be 0 after all body is consumed (decremented before dataHandler) */
+        assert(bodyLengthInDataHandler == 0);
+        std::cout << "POST content-length in dataHandler test PASS" << std::endl;
+    }
+
 }