| Summary: | ServletRequest.getParameterMap() not fully immutable | ||
|---|---|---|---|
| Product: | Tomcat 8 | Reporter: | Woonsan Ko <woonsan> |
| Component: | Catalina | Assignee: | Tomcat Developers Mailing List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | P2 | ||
| Version: | 8.5.x-trunk | ||
| Target Milestone: | ---- | ||
| Hardware: | PC | ||
| OS: | Mac OS X 10.1 | ||
Patch available via PR: https://github.com/apache/tomcat/pull/46 Please review the PR. Thanks, Woonsan 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. 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 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. 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 |
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.