DataBinder should be able to define a different strategy for BeanWrapperImpl how autogrowing should handle gaps in collection properties [SPR-7842]

[spring-projects-issues]

Alex Rau opened SPR-7842 and commented

DataBinder which is used by WebRequestDataBinder and ServletRequestDataBinder grows automatically collections when "autoGrowNestedPaths" is on (AFAIK this is the default for the DataBinder).

The behaviour of DataBinder when "autoGrowNestedPaths" is that a property with a given index N will result in growing a collection to size N if the collection is not large enough. Collection elements with index < N will be initialized with default objects of the collection's type.

Based on WebRequestDataBinder it's therefore easily possible to DoS a web application if it's allowed to bind against a collection property and the client POSTs a single property using a very large index.

Sending a single request containing a parameter for a collection property with index 99999999 caused an OOM on a JVM on MacOsX with default memory settings (64MB?). The list type in this case contained 7 String properties and 2 Longs.

I think there are several things to follow-up:

1. It would be safer to set autoGrowNestedPaths to false by default. Use of DataBinder and autoGrowNestedPaths then would be more restrictive by default and require explicit action to enable autoGrowing.

2. The creation of "default" values in BeanWrapperImpl should be more flexible. A strategy for creating default values would allow clients to define how such default values should be created. In the case of WebRequestDataBinder creating empty (null) collection elements instead of default objects is certainly safer to fill the gaps in the collection - especially for exposed applications to the public. Furthermore this does not expose unwanted restrictions like a maximum allowed index limitation etc.

3. Presumably this could be solved with CustomPropertyEditors. However the majority of developers probably tries to stick with what is available out-of-the box and as the DataBinding in general for "standard" use-cases works fine dealing with the described issue is not obvious and could lead to wide-spread holes.

---

Affects: 3.0.5

Issue Links:

• Can make an OutOfMemoryException by sending a modified form list property with an high index [SPR-8375] #13022 Can make an OutOfMemoryException by sending a modified form list property with an high index ("is duplicated by")
• Data Binder: Auto-grow collections as needed to support nested property binding [SPR-6033] #10702 Data Binder: Auto-grow collections as needed to support nested property binding
• Limit auto grow collection size when using SpEL [SPR-10229] #14862 Limit auto grow collection size when using SpEL

7 votes, 7 watchers

[spring-projects-issues]

Alex Rau commented

The issue introducing autogrowing was #10702.

[spring-projects-issues]

Janning Vygen commented

This issue is very very critical imho. It should be fixed as soon as possible. With autogrowing you can easily crash any webapp using spring with a list form just by sending a very high list index.

[spring-projects-issues]

Alex Rau commented

Is there anything planned on when this issue is going to be fixed ? I guess this issue is a bit underrated in terms of severity - at least in respect to the reporting date and activity. I'm avoiding lists completely right now and use maps instead.

[spring-projects-issues]

Juergen Hoeller commented

We'll be picking this up for 3.1 RC1 and also for the upcoming 3.0.6 release.

Actually, does the OutOfMemoryError cause any long-term damage to the VM in your scenario? I'd expect memory to be made available again right after that exception is being raised... since the allocation of that large chunk of memory clearly failed. In that sort of case, OOMEs should not be fatal but rather indicate a temporary condition only.

Juergen

[spring-projects-issues]

Janning Vygen commented

I can't really remember. But it was something like this: We had some old urls out there which was triggered right after we implemented a behaviour with such lists. So as soon as we deployed our software, we had lots of requests which finally crashed the vm. I don't know why, we just saw OOME fixed the reason and redployed our software.

nice to here, that this going to be fixed. I am still waiting for 3.0.6 because of another small bug but it is pending for long time now.

[spring-projects-issues]

Alex Rau commented

Hi Juergen,

unfortunately I did not analyze if the memory was made available afterwards. I changed the implementation in December in a way that I'm using indexed map entries now which does not have the index gap issue.

I reread my initial issue description and what is missing there is that actually (as just mentioned in the previous sentence) the issue is not directly related to collections per se but afaik list implementations. In contrast maps (and sets as well) are not affected as there's no element position and therefore no definition of gaps between neighbored collection elements as well.

I think the root of the problem is that implicit list elements are instantiated even if never required. Solely an assignment of x[5] would create 6 elements on a list of size 6 no matter if the element 0 to 4 would be ever accessed(read or write).

An alternative would be to size the list to length 6 (again) and only create default elements for positions which are actually accessed (in the above case it's element 5). That was my intention with the "different strategy".

Regarding the OOM: sending excessive requests with high index values affect the system (processes in the same JVM) in a whole - other threads would be unable to request more memory as well and would experience OOMs at the same moment. That could lead to long-term fatal errors in other places and even if the OOM issue described here was a temporary failure or not.

Good news this is going to be fixed in the 3.0.6 release.

[spring-projects-issues]

Juergen Hoeller commented

I eventually resolved this through a newly introduced "autoGrowCollectionLimit" setting on BeanWrapper and DataBinder. This seems to be the easiest solution still, since collections are not guaranteed to accept null values. The default limit in DataBinder is 256 now which should be entirely sufficient for typical web data binding purposes.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Minor correction: With autoGrowNestedPaths=false, we actually do historically try to grow lists but use null values there, failing if null values are not accepted. So semantically, autoGrowNestedPaths=true quite strongly means populating list elements with actual element objects, and the solution above preserves that.

Juergen