Repository: eclipse-vertx/vert.x

Problem statement:
Form upload attribute might get corrupted
The current server form upload processing will create the form multi-map when the HTTP server request ends. Some attributes might use `ByteBuf` that can be recycled by the request decoder overwriting the attribute value. We should capture the request form attribute value as a string to avoid this case.

This issue was found when upgrading to Netty 4.1.52.Final, this version changes the implementation of the post request decoder and this situation happens with very large file upload (32M) and was found in vertx-web-client tests.



Hints:

Patch:
diff --git a/src/main/java/io/vertx/core/http/impl/Http1xServerRequest.java b/src/main/java/io/vertx/core/http/impl/Http1xServerRequest.java
--- a/src/main/java/io/vertx/core/http/impl/Http1xServerRequest.java
+++ b/src/main/java/io/vertx/core/http/impl/Http1xServerRequest.java
@@ -13,12 +13,14 @@
 
 import io.netty.handler.codec.http.*;
 import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.multipart.Attribute;
 import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder;
 import io.netty.handler.codec.http.multipart.InterfaceHttpData;
 import io.vertx.codegen.annotations.Nullable;
 import io.vertx.core.Future;
 import io.vertx.core.Handler;
 import io.vertx.core.MultiMap;
+import io.vertx.core.Promise;
 import io.vertx.core.buffer.Buffer;
 import io.vertx.core.http.*;
 import io.vertx.core.http.Cookie;
@@ -551,6 +553,18 @@ private void onEnd() {
   private void endDecode() {
     try {
       decoder.offer(LastHttpContent.EMPTY_LAST_CONTENT);
+      while (decoder.hasNext()) {
+        InterfaceHttpData data = decoder.next();
+        if (data instanceof Attribute) {
+          Attribute attr = (Attribute) data;
+          try {
+            attributes().add(attr.getName(), attr.getValue());
+          } catch (Exception e) {
+            // Will never happen, anyway handle it somehow just in case
+            handleException(e);
+          }
+        }
+      }
     } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) {
       handleException(e);
     } catch (HttpPostRequestDecoder.EndOfDataDecoderException e) {
diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java b/src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java
--- a/src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java
+++ b/src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java
@@ -16,6 +16,7 @@
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.LastHttpContent;
+import io.netty.handler.codec.http.multipart.Attribute;
 import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder;
 import io.netty.handler.codec.http.multipart.InterfaceHttpData;
 import io.netty.handler.codec.http2.Http2Headers;
@@ -215,6 +216,18 @@ void handleEnd(MultiMap trailers) {
       if (postRequestDecoder != null) {
         try {
           postRequestDecoder.offer(LastHttpContent.EMPTY_LAST_CONTENT);
+          while (postRequestDecoder.hasNext()) {
+            InterfaceHttpData data = postRequestDecoder.next();
+            if (data instanceof Attribute) {
+              Attribute attr = (Attribute) data;
+              try {
+                formAttributes().add(attr.getName(), attr.getValue());
+              } catch (Exception e) {
+                // Will never happen, anyway handle it somehow just in case
+                handleException(e);
+              }
+            }
+          }
         } catch (HttpPostRequestDecoder.EndOfDataDecoderException e) {
           // ignore this as it is expected
         } catch (Exception e) {
diff --git a/src/main/java/io/vertx/core/http/impl/NettyFileUploadDataFactory.java b/src/main/java/io/vertx/core/http/impl/NettyFileUploadDataFactory.java
--- a/src/main/java/io/vertx/core/http/impl/NettyFileUploadDataFactory.java
+++ b/src/main/java/io/vertx/core/http/impl/NettyFileUploadDataFactory.java
@@ -11,13 +11,11 @@
 
 package io.vertx.core.http.impl;
 
-import io.netty.handler.codec.http.HttpConstants;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.multipart.Attribute;
 import io.netty.handler.codec.http.multipart.DefaultHttpDataFactory;
 import io.netty.handler.codec.http.multipart.FileUpload;
 import io.netty.handler.codec.http.multipart.MemoryAttribute;
-import io.vertx.core.Context;
 import io.vertx.core.Handler;
 import io.vertx.core.http.HttpServerFileUpload;
 import io.vertx.core.http.HttpServerRequest;
@@ -78,18 +76,22 @@ public FileUpload createFileUpload(HttpRequest httpRequest, String name, String
     return nettyUpload;
   }
 
-  /**
-   * Vert.x attribute that will add itself to the HTTP request form attributes when completed.
-   */
-  private class VertxAttribute extends MemoryAttribute {
+  private static class VertxAttribute extends MemoryAttribute {
     public VertxAttribute(String name, long definedSize) {
       super(name, definedSize);
       setMaxSize(1024);
     }
+    String value;
     @Override
     protected void setCompleted() {
       super.setCompleted();
-      request.formAttributes().set(getName(), getValue());
+      // Capture value before it gets corrupted
+      // this can be called multiple times (e.g "vert+x" then "vert x"
+      value = super.getValue();
+    }
+    @Override
+    public String getValue() {
+      return value;
     }
   }
 }


Test patch:
diff --git a/src/test/java/io/vertx/core/http/Http2ServerTest.java b/src/test/java/io/vertx/core/http/Http2ServerTest.java
--- a/src/test/java/io/vertx/core/http/Http2ServerTest.java
+++ b/src/test/java/io/vertx/core/http/Http2ServerTest.java
@@ -649,7 +649,7 @@ public void testPostFileUpload() throws Exception {
         });
       });
       req.endHandler(v -> {
-        assertEquals(5, req.formAttributes().size());
+        assertEquals(0, req.formAttributes().size());
         req.response().putHeader("content-type", "text/plain").end("done");
       });
     });
diff --git a/src/test/java/io/vertx/core/http/HttpServerFileUploadTest.java b/src/test/java/io/vertx/core/http/HttpServerFileUploadTest.java
--- a/src/test/java/io/vertx/core/http/HttpServerFileUploadTest.java
+++ b/src/test/java/io/vertx/core/http/HttpServerFileUploadTest.java
@@ -26,6 +26,8 @@
 import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
 import java.net.URLEncoder;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -325,11 +327,7 @@ private void testFormUploadFile(String filename,
               resp.bodyHandler(body -> {
                 assertEquals(0, body.length());
               });
-              if (cancelStream) {
-                assertEquals(0, attributeCount.get());
-              } else {
-                assertEquals(3, attributeCount.get());
-              }
+              assertEquals(0, attributeCount.get());
               complete();
             }));
           }
@@ -354,8 +352,16 @@ public void testFormUploadAttributes() throws Exception {
           attributeCount.set(attrs.size());
           assertEquals("vert x", attrs.get("framework"));
           assertEquals("vert x", req.getFormAttribute("framework"));
+          assertEquals("vert x", req.formAttributes().get("framework"));
+          assertEquals(Collections.singletonList("vert x"), req.formAttributes().getAll("framework"));
           assertEquals("jvm", attrs.get("runson"));
           assertEquals("jvm", req.getFormAttribute("runson"));
+          assertEquals("jvm", req.formAttributes().get("runson"));
+          assertEquals(Collections.singletonList("jvm"), req.formAttributes().getAll("runson"));
+          assertEquals("0", attrs.get("list"));
+          assertEquals("0", req.getFormAttribute("list"));
+          assertEquals("0", req.formAttributes().get("list"));
+          assertEquals(Arrays.asList("0", "1"), req.formAttributes().getAll("list"));
           req.response().end();
         });
       }
@@ -363,7 +369,12 @@ public void testFormUploadAttributes() throws Exception {
 
     Buffer buffer = Buffer.buffer();
     // Make sure we have one param that needs url encoding
-    buffer.appendString("framework=" + URLEncoder.encode("vert x", "UTF-8") + "&runson=jvm", "UTF-8");
+    buffer.appendString(
+      "framework=" + URLEncoder.encode("vert x", "UTF-8") +
+      "&runson=jvm" +
+      "&list=0" +
+      "&list=1"
+      , "UTF-8");
     server.listen(testAddress, onSuccess(s -> {
       client.request(new RequestOptions(requestOptions)
         .setMethod(HttpMethod.POST)
@@ -377,7 +388,7 @@ public void testFormUploadAttributes() throws Exception {
             resp.bodyHandler(body -> {
               assertEquals(0, body.length());
             });
-            assertEquals(2, attributeCount.get());
+            assertEquals(3, attributeCount.get());
             testComplete();
           }));
       }));