|Summary:||ServletRequest.getParameterMap() not fully immutable|
|Product:||Tomcat 8||Reporter:||Woonsan Ko <woonsan>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
|OS:||Mac OS X 10.1|
Description Woonsan Ko 2017-03-03 00:49:00 UTC
A corner case was discovered: if request.getParameterMap().keySet().retainAll(set) is called even after the internal ParameterMap is locked. Reproducible step: - Create a simple JSP page like this: <% // test.jsp out.println("request.getParameterMap(): " + request.getParameterMap()); request.getParameterMap().keySet().retainAll(java.util.Collections.emptySet()); out.println("request.getParameterMap(): " + request.getParameterMap()); %> - Request the page. e.g, http://localhost:8080/examples/test.jsp?p1=v1&p2=v2 - You will notice the second output is empty. According to the javadoc of javax.servlet.ServletRequest#getParameterMap(), the returned map must be immutable. But, its #keySet() seems to return a mutable set. It could have returned an unmodifiable set at least like Collections.unmodifiableMap(map) does.
Comment 1 Woonsan Ko 2017-03-03 07:14:30 UTC
Patch available via PR: https://github.com/apache/tomcat/pull/46 Please review the PR. Thanks, Woonsan
Comment 2 Mark Thomas 2017-03-03 11:23:53 UTC
Generally, I think it would be safer (in terms of future additions to Map and changes to the implementation of LinkedHashMap) and probably require less code if ParameterMap used composition rather than extension.
Comment 3 Woonsan Ko 2017-03-03 16:32:43 UTC
Hi Mark, Yes, that makes more sense to use containment instead of extending. Even if newer JVM has more new API support in the future, ParameterMap implementing j.u.Map with a contained delegate map should be a lot safer in any case. I'll create a new pull request using containment soon and update it here. Regards, Woonsan
Comment 4 Woonsan Ko 2017-03-03 18:49:13 UTC
I've created a new PR: https://github.com/apache/tomcat/pull/47, following Mark's suggestion to use containment instead of inheritance. Indeed, it helps reduce code size and safer in the future changes in JVM. Please take a review.
Comment 5 Mark Thomas 2017-03-06 15:38:03 UTC
Thanks for the report and the patch. I took the opportunity to do some additional clean-up at the same time - particularly of the Javadoc. Fixed in: - trunk for 9.0.0.M18 onwards - 8.5.x for 8.5.12 onwards - 8.0.x for 8.0.42 onwards - 7.0.x for 7.0.76 onwards