Bug 60808 - ServletRequest.getParameterMap() not fully immutable
Summary: ServletRequest.getParameterMap() not fully immutable
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: PC Mac OS X 10.1
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-03 00:49 UTC by Woonsan Ko
Modified: 2017-03-06 15:38 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
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