Fixes #3856 - Different behaviour with maxFormContentSize=0 if Content-Length header is present/missing.

Updated code to reflect reviews.
Now lookup of system properties and server attributes is done in
ContextHandler.doStart(), so that the getter always return the
actual value (and this is good for JMX too).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

@@ -498,8 +498,8 @@ public class Request implements HttpServletRequest
     {
         try
         {
-            int maxFormContentSize = -1;
+            int maxFormContentSize = ContextHandler.DEFAULT_MAX_FORM_CONTENT_SIZE;
-            int maxFormKeys = -1;
+            int maxFormKeys = ContextHandler.DEFAULT_MAX_FORM_KEYS;
 
             if (_context != null)
             {
@@ -508,30 +508,8 @@ public class Request implements HttpServletRequest
                 maxFormKeys = contextHandler.getMaxFormKeys();
             }
 
-            if (maxFormContentSize < 0)
-            {
-                Object obj = _channel.getServer().getAttribute("org.eclipse.jetty.server.Request.maxFormContentSize");
-                if (obj instanceof Number)
-                    maxFormContentSize = ((Number)obj).intValue();
-                else if (obj instanceof String)
-                    maxFormContentSize = Integer.parseInt((String)obj);
-                if (maxFormContentSize < 0)
-                    maxFormContentSize = 200000;
-            }
-
-            if (maxFormKeys < 0)
-            {
-                Object obj = _channel.getServer().getAttribute("org.eclipse.jetty.server.Request.maxFormKeys");
-                if (obj instanceof Number)
-                    maxFormKeys = ((Number)obj).intValue();
-                else if (obj instanceof String)
-                    maxFormKeys = Integer.parseInt((String)obj);
-                if (maxFormKeys < 0)
-                    maxFormKeys = 1000;
-            }
-
             int contentLength = getContentLength();
-            if (contentLength > maxFormContentSize)
+            if (maxFormContentSize >= 0 && contentLength > maxFormContentSize)
                 throw new IllegalStateException("Form is larger than max length " + maxFormContentSize);
 
             InputStream in = getInputStream();
@@ -672,9 +650,21 @@ public class Request implements HttpServletRequest
         MetaData.Request metadata = _metaData;
         if (metadata == null)
             return -1;
-        if (metadata.getContentLength() != Long.MIN_VALUE)
+
-            return (int)metadata.getContentLength();
+        long contentLength = metadata.getContentLength();
-        return (int)metadata.getFields().getLongField(HttpHeader.CONTENT_LENGTH.toString());
+        if (contentLength != Long.MIN_VALUE)
+        {
+            if (contentLength > Integer.MAX_VALUE)
+            {
+                // Per ServletRequest#getContentLength() javadoc this must return -1 for values exceeding Integer.MAX_VALUE
+                return -1;
+            }
+            else
+            {
+                return (int)contentLength;
+            }
+        }
+        return (int)metadata.getFields().getLongField(HttpHeader.CONTENT_LENGTH.asString());
     }
 
     /*
@@ -688,7 +678,7 @@ public class Request implements HttpServletRequest
             return -1L;
         if (metadata.getContentLength() != Long.MIN_VALUE)
             return metadata.getContentLength();
-        return metadata.getFields().getLongField(HttpHeader.CONTENT_LENGTH.toString());
+        return metadata.getFields().getLongField(HttpHeader.CONTENT_LENGTH.asString());
     }
 
     public long getContentRead()

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java

@@ -140,6 +140,9 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
      * for the attribute value.
      */
     public static final String MANAGED_ATTRIBUTES = "org.eclipse.jetty.server.context.ManagedAttributes";
+    private static final int NOT_INITIALIZED = -2;
+    public static final int DEFAULT_MAX_FORM_CONTENT_SIZE = 200000;
+    public static final int DEFAULT_MAX_FORM_KEYS = 1000;
 
     /**
      * Get the current ServletContext implementation.
@@ -192,8 +195,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
 
     private Logger _logger;
     private boolean _allowNullPathInfo;
-    private int _maxFormKeys = Integer.getInteger("org.eclipse.jetty.server.Request.maxFormKeys", -1);
+    private int _maxFormKeys = NOT_INITIALIZED;
-    private int _maxFormContentSize = Integer.getInteger("org.eclipse.jetty.server.Request.maxFormContentSize", -1);
+    private int _maxFormContentSize = NOT_INITIALIZED;
     private boolean _compactPath = false;
     private boolean _usingSecurityManager = System.getSecurityManager() != null;
 
@@ -782,15 +785,19 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
     @Override
     protected void doStart() throws Exception
     {
+        if (_maxFormKeys == NOT_INITIALIZED)
+            _maxFormKeys = lookup("org.eclipse.jetty.server.Request.maxFormKeys", DEFAULT_MAX_FORM_KEYS);
+
+        if (_maxFormContentSize == NOT_INITIALIZED)
+            _maxFormContentSize = lookup("org.eclipse.jetty.server.Request.maxFormContentSize", DEFAULT_MAX_FORM_CONTENT_SIZE);
+
         _availability = Availability.STARTING;
 
         if (_contextPath == null)
             throw new IllegalStateException("Null contextPath");
 
         if (_logger == null)
-        {
             _logger = Log.getLogger(ContextHandler.class.getName() + getLogNameSuffix());
-        }
 
         ClassLoader oldClassloader = null;
         Thread currentThread = null;
@@ -860,6 +867,22 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
         return '.' + logName.replaceAll("\\W", "_");
     }
 
+    private int lookup(String key, int dftValue)
+    {
+        Integer value = Integer.getInteger(key);
+        if (value == null)
+        {
+            Object attribute = getServer().getAttribute(key);
+            if (attribute instanceof Number)
+                value = ((Number)attribute).intValue();
+            else if (attribute instanceof String)
+                value = Integer.parseInt((String)attribute);
+            else
+                value = dftValue;
+        }
+        return value;
+    }
+
     /**
      * Extensible startContext. this method is called from {@link ContextHandler#doStart()} instead of a call to super.doStart(). This allows derived classes to
      * insert additional handling (Eg configuration) before the call to super.doStart by this method will start contained handlers.

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/FormTest.java

@@ -18,26 +18,39 @@
 
 package org.eclipse.jetty.servlet;
 
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
 import java.util.function.Function;
+import java.util.stream.Stream;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.client.util.DeferredContentProvider;
 import org.eclipse.jetty.client.util.FormContentProvider;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.http.MimeTypes;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.util.Fields;
 import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 public class FormTest
 {
+    private static final int MAX_FORM_CONTENT_SIZE = 128;
+    private static final int MAX_FORM_KEYS = 4;
+
     private Server server;
     private ServerConnector connector;
     private HttpClient client;
@@ -78,23 +91,28 @@ public class FormTest
             server.stop();
     }
 
-    @Test
+    public static Stream<Arguments> formContentSizeScenarios()
-    public void testMaxFormContentSizeZeroWithoutContentLength() throws Exception
     {
-        testMaxFormContentSizeZero(false);
+        return Stream.of(
+            Arguments.of(null, true),
+            Arguments.of(null, false),
+            Arguments.of(-1, true),
+            Arguments.of(-1, false),
+            Arguments.of(0, true),
+            Arguments.of(0, false),
+            Arguments.of(MAX_FORM_CONTENT_SIZE, true),
+            Arguments.of(MAX_FORM_CONTENT_SIZE, false)
+        );
     }
 
-    @Test
+    @ParameterizedTest
-    public void testMaxFormContentSizeZeroWithContentLength() throws Exception
+    @MethodSource("formContentSizeScenarios")
-    {
+    public void testMaxFormContentSizeExceeded(Integer maxFormContentSize, boolean withContentLength) throws Exception
-        testMaxFormContentSizeZero(true);
-    }
-
-    private void testMaxFormContentSizeZero(boolean addContentLength) throws Exception
     {
         start(handler ->
         {
-            handler.setMaxFormContentSize(0);
+            if (maxFormContentSize != null)
+                handler.setMaxFormContentSize(maxFormContentSize);
             return new HttpServlet()
             {
                 @Override
@@ -105,30 +123,52 @@ public class FormTest
             };
         });
 
-        Fields formParams = new Fields();
+        byte[] key = "foo=".getBytes(StandardCharsets.US_ASCII);
-        formParams.add("foo", "bar");
+        int length = (maxFormContentSize == null || maxFormContentSize < 0)
+            ? ContextHandler.DEFAULT_MAX_FORM_CONTENT_SIZE
+            : maxFormContentSize;
+        // Avoid empty value.
+        length = length + 1;
+        byte[] value = new byte[length];
+        Arrays.fill(value, (byte)'x');
+        DeferredContentProvider content = new DeferredContentProvider(ByteBuffer.wrap(key), ByteBuffer.wrap(value));
+
         ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
             .method(HttpMethod.POST)
             .path(contextPath + servletPath)
-            .content(new FormContentProvider(formParams)
+            .header(HttpHeader.CONTENT_TYPE, MimeTypes.Type.FORM_ENCODED.asString())
+            .content(content)
+            .onRequestBegin(request ->
             {
-                @Override
+                if (withContentLength)
-                public long getLength()
+                    content.close();
+            })
+            .onRequestCommit(request ->
             {
-                    return addContentLength ? super.getLength() : -1;
+                if (!withContentLength)
-                }
+                    content.close();
             })
             .send();
 
-        assertEquals(HttpStatus.BAD_REQUEST_400, response.getStatus());
+        int expected = (maxFormContentSize != null && maxFormContentSize < 0)
+            ? HttpStatus.OK_200
+            : HttpStatus.BAD_REQUEST_400;
+        assertEquals(expected, response.getStatus());
     }
 
-    @Test
+    public static Stream<Integer> formKeysScenarios()
-    public void testMaxFormKeysZero() throws Exception
+    {
+        return Stream.of(null, -1, 0, MAX_FORM_KEYS);
+    }
+
+    @ParameterizedTest
+    @MethodSource("formKeysScenarios")
+    public void testMaxFormKeysExceeded(Integer maxFormKeys) throws Exception
     {
         start(handler ->
         {
-            handler.setMaxFormKeys(0);
+            if (maxFormKeys != null)
+                handler.setMaxFormKeys(maxFormKeys);
             return new HttpServlet()
             {
                 @Override
@@ -139,14 +179,25 @@ public class FormTest
             };
         });
 
+        int keys = (maxFormKeys == null || maxFormKeys < 0)
+            ? ContextHandler.DEFAULT_MAX_FORM_KEYS
+            : maxFormKeys;
+        // Have at least one key.
+        keys = keys + 1;
         Fields formParams = new Fields();
-        formParams.add("foo", "bar");
+        for (int i = 0; i < keys; ++i)
+        {
+            formParams.add("key_" + i, "value_" + i);
+        }
         ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
             .method(HttpMethod.POST)
             .path(contextPath + servletPath)
             .content(new FormContentProvider(formParams))
             .send();
 
-        assertEquals(HttpStatus.BAD_REQUEST_400, response.getStatus());
+        int expected = (maxFormKeys != null && maxFormKeys < 0)
+            ? HttpStatus.OK_200
+            : HttpStatus.BAD_REQUEST_400;
+        assertEquals(expected, response.getStatus());
     }
 }

jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java

@@ -232,7 +232,7 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
         synchronized (map)
         {
             String key = null;
-            String value = null;
+            String value;
             int mark = -1;
             boolean encoded = false;
             for (int i = 0; i < content.length(); i++)
@@ -384,9 +384,9 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
      *
      * @param in InputSteam to read
      * @param map MultiMap to add parameters to
-     * @param maxLength maximum length of form to read
+     * @param maxLength maximum length of form to read or -1 for no limit
-     * @param maxKeys maximum number of keys to read
+     * @param maxKeys maximum number of keys to read or -1 for no limit
-     * @throws IOException if unable to decode InputStream as ISO8859-1
+     * @throws IOException if unable to decode the InputStream as ISO8859-1
      */
     public static void decode88591To(InputStream in, MultiMap<String> map, int maxLength, int maxKeys)
         throws IOException
@@ -457,7 +457,6 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
             {
                 map.add(buffer.toString(), "");
             }
-
             checkMaxKeys(map, maxKeys);
         }
     }
@@ -467,9 +466,9 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
      *
      * @param in InputSteam to read
      * @param map MultiMap to add parameters to
-     * @param maxLength maximum form length to decode
+     * @param maxLength maximum form length to decode or -1 for no limit
-     * @param maxKeys the maximum number of keys to read
+     * @param maxKeys the maximum number of keys to read or -1 for no limit
-     * @throws IOException if unable to decode input stream
+     * @throws IOException if unable to decode the input stream
      */
     public static void decodeUtf8To(InputStream in, MultiMap<String> map, int maxLength, int maxKeys)
         throws IOException
@@ -540,7 +539,6 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
             {
                 map.add(buffer.toReplacedString(), "");
             }
-
             checkMaxKeys(map, maxKeys);
         }
     }
@@ -561,9 +559,9 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
      * @param in the stream containing the encoded parameters
      * @param map the MultiMap to decode into
      * @param charset the charset to use for decoding
-     * @param maxLength the maximum length of the form to decode
+     * @param maxLength the maximum length of the form to decode or -1 for no limit
-     * @param maxKeys the maximum number of keys to decode
+     * @param maxKeys the maximum number of keys to decode or -1 for no limit
-     * @throws IOException if unable to decode input stream
+     * @throws IOException if unable to decode the input stream
      */
     public static void decodeTo(InputStream in, MultiMap<String> map, String charset, int maxLength, int maxKeys)
         throws IOException
@@ -689,7 +687,6 @@ public class UrlEncoded extends MultiMap<String> implements Cloneable
                 {
                     map.add(output.toString(charset), "");
                 }
-
                 checkMaxKeys(map, maxKeys);
             }
         }