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

[sbordet]

#3856.

Fixed zero checks and exception messages.

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

[gregw]

Can you explain in this PR what the actual fix is? I can see lots of code cleanup, but it is not apparent which part actually is the fix - specially as I can see no http2 specific code? So a description in this PR (that end up in squashed commit) would be much appreciated.

[joakime]

0 means a value of zero.
-1 means unlimited (everywhere else in our code).

simone tried to fix that here, but the use of -1 to mean unset / undefined makes things difficult for the cascade of values per request found in the Request.extractFormParameters method.

making ContextHandler.getMaxFormKeys and ContextHandler.getMaxFormSize return sane values on JMX was also part of this effort.

If the Request.extractFormParameters didn't have to interrogate the Server every time for possible max value changes this PR would be simpler (just do the calculation of max keys and max form size on ContextHandler.doStart() only). But that has the potential to break backward compatibility.

[gregw]

@joakime I still don't understand exactly what it the bug and what is the fix. I see lots of code fiddling with how we handle 0 and -1, but it is not clear to me what this PR is actually doing. Note that we often use -1 to indicate "set a default value from heuristics" , which I think is what Simone is doing here. I think we also sometimes use 0 to mean unlimited elsewhere

So if 0 means 0, is the intention to use max-value to mean no limit? That is a behaviour change.

Anyway, if somebody can actually explain what the bug/fix is, then we can talk about what the cleanup should be.

[sbordet]

@gregw this issue was not about HTTP/2 - the OP was mistaken about that.
So it turned out to be what the issue title now says, a different behavior of maxFormContentSize with respect to Content-Length.

As such, the fixes are primarily checkMaxKeys() and checkMaxLength() now also checking for >=0 not only >0.

Another problem was the meaning of -1 and 0 for those properties. -1 actually meant "use the default" and 0 was not handled properly (different behavior depending on whether the Content-Length was there).

The current solution is -1 for the default (to keep backwards compatibility), 0 means zero (so it basically disables form uploading), and any positive means it; for unlimited values you have to explicitly specify a large number.

We may break people that specified 0 thinking that meant "unlimited" but hopefully not many did that.

JMX is still "broken" in that the getter will show -1 while instead a default value will be used (but it's hardcoded, so there is no visualization of what the default value is); however it has always been so.

So basically this issue was all about a bad pattern for initializing those properties, using -1 to mean "default" and not handling the -1 and 0 corner cases properly.

[gregw]

So why can't we just change to consistently interpreting 0 as unlimited (with or without content-length) and thus keep the behaviour ?

I also think that it would be a reasonable change to always set the values in the ContextHandler (from server defaults) in doStart. Sure that means that we can't dynamically change them by changing server values on the fly, but we can change them dynamically on the context, so I think that is OK

[joakime]

So if I want to set maxKeys and maxFormSize to 0 (meaning no forms) what do I do?
(yes, there are people that want this, as they consider the Servlet getParameters() logic to just be too much of a hassle with Filters and 3rd party libraries, and would rather force the developers to implement things in other ways)

[gregw]

@joakime I guess that if we had our time over, then making 0 mean 0 would have been the way to do that. But we didn't and 0 meant unlimited, so I don't see how we can change that in anything less than a major release like 10.0.0

...

OR

Currently each context as constructed looks for:

    private int _maxFormKeys = Integer.getInteger("org.eclipse.jetty.server.Request.maxFormKeys", -1).intValue();
    private int _maxFormContentSize = Integer.getInteger("org.eclipse.jetty.server.Request.maxFormContentSize", -1).intValue();

These values can then be mutated with setters. If by the time a request is processed and they are still -1, then we look in server attributes:

_channel.getServer().getAttribute("org.eclipse.jetty.server.Request.maxFormContentSize");
_channel.getServer().getAttribute("org.eclipse.jetty.server.Request.maxFormKeys");

only if these are not found do we use default values. So currently VERY few users would ever actually set a -1 value and it is mostly used as an internal not-set flag. If users want to dynamically change, they will mostly like via the context setters and not the server attributes.

So I think that it is a low risk change to set the values during construction with:

    private int _maxFormKeys = Integer.getInteger("org.eclipse.jetty.server.Request.maxFormKeys", 1000).intValue();
    private boolean _maxFormKeysSet;
    private int _maxFormContentSize = Integer.getInteger("org.eclipse.jetty.server.Request.maxFormContentSize", 200000).intValue();
    private boolean _maxFormContentSizeSet;

Note the booleans that we will set true if anybody calls the setters. Then during doStart we will check the server attributes and possibly override the defaults only if the booleans are still false. With this setup, we don't use -1, so we could make that mean unlimited and 0 could mean 0. Any users that currently set 0 expecting to get no limit will quickly find out the problem and can then explicitly set either a large number or -1

HOWEVER - ideally these could be set via HttpConfiguration, so you could potentially have different sizes for different connectors.... hmmm is that overkill? But even if it is, we could still achieve that without using the -1. As each request is handled, we could ask the context if the value was set or not. If it is not set, we would look for a default value in the HttpConfiguration. Hmm getting complex, but possible

tl;dr; changing to 0 means 0 within the current setup is not good. But if more substantial change was made to the configuration that allowed the -1 to be re-purposed to mean no limit , then I could accept a change to 0 means 0 now.

[sbordet]

@gregw @joakime I have updated the code to be "correct".

The previous behavior for maxFormContentSize was that maxFormContentSize==0 meant unlimited, but only if the Content-Length header was present. In case of chunked transfer, 0 meant forms were not allowed.

The previous behavior for maxFormKeys was that maxFormKeys==0meant unlimited, and the check was wrong for maxFormKeys>0 (setting it to N would have allowed N+1 keys).

Now the behavior is identical for the 2 parameters and it is:

• unset -> default limit
• -1 -> unlimited
• 0 -> not allowed
• N -> limited to N

So we are potentially breaking people that meant 0 as unlimited.
It may be ok to break with the intention to raise awareness.

[sbordet]

@joakime @gregw now we have a test failure, where the test was not using a ContextHandler, but just setting the form limits as Server attributes.

So apparently we really need to re-read those attributes for every attempt to use the parameters API, not just at ContextHandler start.

[sbordet]

@joakime @gregw changed again the logic to read the attributes from the Server if the ContextHandler is missing. This fixes the failing test.

Behavior did not change WRT to the past fixes (-1 -> unlimited, 0 -> not allowed) so we may break people that meant 0 as unlimited before the fixes.

[joakime]

Behavior did not change (-1 -> unlimited, 0 -> not allowed) and we may break people that meant 0 as unlimited.

@WalkerWatch - This tidbit will be useful to mention in the jetty-announce mails.

@sbordet so is the ContextHandler now the official truth of those settings?
If so, does the javadoc reflect this new reality with -1 and 0?

[sbordet]

@joakime yes now ContextHandler is the component you want to use to configure those values.
If you don't have a ContextHandler (e.g. you are using a plain AbstractHandler subclass) then you can configure those values via Server attributes.
Javadocs are updated.

[gregw]

LGTM

[gregw]

I like this code... very obvious what it is doing.

[gregw]

duplicated code.... but not a big deal.

[sbordet]

Actually dead code now, I removed it.

[jglick]

Note that this is incompatible as seen in jenkinsci/winstone@8a8a146.

[joakime]

Opened Issue #4638