Document & fix PROXY regression/bug

src/HttpParser.h

@@ -34,7 +34,7 @@
 namespace uWS {
 
 /* We require at least this much post padding */
-static const int MINIMUM_HTTP_POST_PADDING = 32;
+static const unsigned int MINIMUM_HTTP_POST_PADDING = 32;
 
 struct HttpRequest {
 
@@ -188,7 +188,7 @@ private:
 
     // the only caller of getHeaders
     template <int CONSUME_MINIMALLY>
-    std::pair<int, void *> fenceAndConsumePostPadded(char *data, unsigned int length, void *user, void *reserved, HttpRequest *req, fu2::unique_function<void *(void *, HttpRequest *)> &requestHandler, fu2::unique_function<void *(void *, std::string_view, bool)> &dataHandler) {
+    std::pair<unsigned int, void *> fenceAndConsumePostPadded(char *data, unsigned int length, void *user, void *reserved, HttpRequest *req, fu2::unique_function<void *(void *, HttpRequest *)> &requestHandler, fu2::unique_function<void *(void *, std::string_view, bool)> &dataHandler) {
 
         /* How much data we CONSUMED (to throw away) */
         unsigned int consumedTotal = 0;
@@ -198,8 +198,11 @@ private:
         ProxyParser *pp = (ProxyParser *) reserved;
 
         /* Parse PROXY protocol */
-        auto [done, offset] = pp->parse({data, /*(unsigned int)*/ length});
+        auto [done, offset] = pp->parse({data, length});
         if (!done) {
+            /* We do not reset the ProxyParser (on filure) since it is tied to this
+             * connection, which is really only supposed to ever get one PROXY frame
+             * anyways. We do however allow multiple PROXY frames to be sent (overwrites former). */
             return {0, user};
         } else {
             /* We have consumed this data so skip it */
@@ -212,6 +215,9 @@ private:
         (void) reserved;
 #endif
 
+        /* For tracking whether we managed to parse more than just PROXY */
+        unsigned int consumedProxyParser = consumedTotal;
+
         /* Fence one byte past end of our buffer (buffer has post padded margins) */
         data[length] = '\r';
 
@@ -266,6 +272,15 @@ private:
                 break;
             }
         }
+
+        /* It is critical for fallback buffering logic that we only return with success
+         * if we managed to parse a complete HTTP request. Returning success for PROXY means
+         * we can end up succeeding, yet leaving bytes in the fallback buffer which is then
+         * removed, and our counters to flip due to overflow and we end up with a crash */
+        if (consumedProxyParser == consumedTotal) {
+            return {0, user};
+        }
+
         return {consumedTotal, user};
     }
 
@@ -281,7 +296,7 @@ public:
             // this is exactly the same as below!
             // todo: refactor this
             if (remainingStreamingBytes >= length) {
-                void *returnedUser = dataHandler(user, std::string_view(data, length), remainingStreamingBytes == (unsigned int) length);
+                void *returnedUser = dataHandler(user, std::string_view(data, length), remainingStreamingBytes == length);
                 remainingStreamingBytes -= length;
                 return returnedUser;
             } else {
@@ -298,26 +313,28 @@ public:
             }
 
         } else if (fallback.length()) {
-            int had = (int) fallback.length();
+            unsigned int had = (unsigned int) fallback.length();
 
             size_t maxCopyDistance = std::min(MAX_FALLBACK_SIZE - fallback.length(), (size_t) length);
 
             /* We don't want fallback to be short string optimized, since we want to move it */
-            fallback.reserve(fallback.length() + maxCopyDistance + std::max<int>(MINIMUM_HTTP_POST_PADDING, sizeof(std::string)));
+            fallback.reserve(fallback.length() + maxCopyDistance + std::max<unsigned int>(MINIMUM_HTTP_POST_PADDING, sizeof(std::string)));
             fallback.append(data, maxCopyDistance);
 
             // break here on break
-            std::pair<int, void *> consumed = fenceAndConsumePostPadded<true>(fallback.data(), (unsigned int) fallback.length(), user, reserved, &req, requestHandler, dataHandler);
+            std::pair<unsigned int, void *> consumed = fenceAndConsumePostPadded<true>(fallback.data(), (unsigned int) fallback.length(), user, reserved, &req, requestHandler, dataHandler);
             if (consumed.second != user) {
                 return consumed.second;
             }
 
             if (consumed.first) {
 
+                /* This logic assumes that we consumed everything in fallback buffer.
+                 * This is critically important, as we will get an integer overflow in case
+                 * of "had" being larger than what we consumed, and that we would drop data */
                 fallback.clear();
-
-                data += (unsigned int) (consumed.first - had);
-                length -= (unsigned int) (consumed.first - had);
+                data += consumed.first - had;
+                length -= consumed.first - had;
 
                 if (remainingStreamingBytes) {
                     // this is exactly the same as above!
@@ -349,16 +366,16 @@ public:
             }
         }
 
-        std::pair<int, void *> consumed = fenceAndConsumePostPadded<false>(data, length, user, reserved, &req, requestHandler, dataHandler);
+        std::pair<unsigned int, void *> consumed = fenceAndConsumePostPadded<false>(data, length, user, reserved, &req, requestHandler, dataHandler);
         if (consumed.second != user) {
             return consumed.second;
         }
 
-        data += (unsigned int) consumed.first;
-        length -= (unsigned int) consumed.first;
+        data += consumed.first;
+        length -= consumed.first;
 
         if (length) {
-            if ((unsigned int) length < MAX_FALLBACK_SIZE) {
+            if (length < MAX_FALLBACK_SIZE) {
                 fallback.append(data, length);
             } else {
                 return errorHandler(user);