Author: markt Date: Mon Mar 6 15:28:47 2017 New Revision: 1785667 URL: http://svn.apache.org/viewvc?rev=1785667&view=rev Log: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=60808 Ensure that the Map returned by ServletRequest.getParameterMap() is fully immutable. Based on a patch provided by woosan.
Added: tomcat/trunk/test/org/apache/catalina/util/TestParameterMap.java Modified: tomcat/trunk/java/org/apache/catalina/util/ParameterMap.java tomcat/trunk/webapps/docs/changelog.xml Modified: tomcat/trunk/java/org/apache/catalina/util/ParameterMap.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/util/ParameterMap.java?rev=1785667&r1=1785666&r2=1785667&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/util/ParameterMap.java (original) +++ tomcat/trunk/java/org/apache/catalina/util/ParameterMap.java Mon Mar 6 15:28:47 2017 @@ -16,13 +16,17 @@ */ package org.apache.catalina.util; +import java.io.Serializable; +import java.util.Collection; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; import org.apache.tomcat.util.res.StringManager; /** - * Extended implementation of <strong>HashMap</strong> that includes a + * Implementation of <strong>java.util.Map</strong> that includes a * <code>locked</code> property. This class can be used to safely expose * Catalina internal parameter map objects to user classes without having * to clone them in order to avoid modifications. When first created, a @@ -33,20 +37,22 @@ import org.apache.tomcat.util.res.String * * @author Craig R. McClanahan */ -public final class ParameterMap<K,V> extends LinkedHashMap<K,V> { +public final class ParameterMap<K,V> implements Map<K,V>, Serializable { - private static final long serialVersionUID = 1L; + private static final long serialVersionUID = 2L; + + private final Map<K,V> delegatedMap; + + private final Map<K,V> unmodifiableDelegatedMap; - // ----------------------------------------------------------- Constructors /** * Construct a new, empty map with the default initial capacity and * load factor. */ public ParameterMap() { - - super(); - + delegatedMap = new LinkedHashMap<>(); + unmodifiableDelegatedMap = Collections.unmodifiableMap(delegatedMap); } @@ -57,9 +63,8 @@ public final class ParameterMap<K,V> ext * @param initialCapacity The initial capacity of this map */ public ParameterMap(int initialCapacity) { - - super(initialCapacity); - + delegatedMap = new LinkedHashMap<>(initialCapacity); + unmodifiableDelegatedMap = Collections.unmodifiableMap(delegatedMap); } @@ -71,9 +76,8 @@ public final class ParameterMap<K,V> ext * @param loadFactor The load factor of this map */ public ParameterMap(int initialCapacity, float loadFactor) { - - super(initialCapacity, loadFactor); - + delegatedMap = new LinkedHashMap<>(initialCapacity, loadFactor); + unmodifiableDelegatedMap = Collections.unmodifiableMap(delegatedMap); } @@ -83,15 +87,11 @@ public final class ParameterMap<K,V> ext * @param map Map whose contents are duplicated in the new map */ public ParameterMap(Map<K,V> map) { - - super(map); - + delegatedMap = new LinkedHashMap<>(map); + unmodifiableDelegatedMap = Collections.unmodifiableMap(delegatedMap); } - // ------------------------------------------------------------- Properties - - /** * The current lock state of this parameter map. */ @@ -102,9 +102,7 @@ public final class ParameterMap<K,V> ext * @return the locked state of this parameter map. */ public boolean isLocked() { - - return (this.locked); - + return locked; } @@ -114,102 +112,145 @@ public final class ParameterMap<K,V> ext * @param locked The new locked state */ public void setLocked(boolean locked) { - this.locked = locked; - } /** * The string manager for this package. */ - private static final StringManager sm = - StringManager.getManager("org.apache.catalina.util"); - - - // --------------------------------------------------------- Public Methods - + private static final StringManager sm = StringManager.getManager("org.apache.catalina.util"); /** - * Remove all mappings from this map. + * {@inheritDoc} * * @exception IllegalStateException if this map is currently locked */ @Override public void clear() { - - if (locked) - throw new IllegalStateException - (sm.getString("parameterMap.locked")); - super.clear(); - + checkLocked(); + delegatedMap.clear(); } /** - * Associate the specified value with the specified key in this map. If - * the map previously contained a mapping for this key, the old value is - * replaced. - * - * @param key Key with which the specified value is to be associated - * @param value Value to be associated with the specified key - * - * @return The previous value associated with the specified key, or - * <code>null</code> if there was no mapping for key + * {@inheritDoc} * * @exception IllegalStateException if this map is currently locked */ @Override public V put(K key, V value) { - - if (locked) - throw new IllegalStateException - (sm.getString("parameterMap.locked")); - return (super.put(key, value)); - + checkLocked(); + return delegatedMap.put(key, value); } /** - * Copy all of the mappings from the specified map to this one. These - * mappings replace any mappings that this map had for any of the keys - * currently in the specified Map. - * - * @param map Mappings to be stored into this map + * {@inheritDoc} * * @exception IllegalStateException if this map is currently locked */ @Override public void putAll(Map<? extends K,? extends V> map) { - - if (locked) - throw new IllegalStateException - (sm.getString("parameterMap.locked")); - super.putAll(map); - + checkLocked(); + delegatedMap.putAll(map); } /** - * Remove the mapping for this key from the map if present. - * - * @param key Key whose mapping is to be removed from the map - * - * @return The previous value associated with the specified key, or - * <code>null</code> if there was no mapping for that key + * {@inheritDoc} * * @exception IllegalStateException if this map is currently locked */ @Override public V remove(Object key) { + checkLocked(); + return delegatedMap.remove(key); + } + + + private void checkLocked() { + if (locked) { + throw new IllegalStateException(sm.getString("parameterMap.locked")); + } + } + + + @Override + public int size() { + return delegatedMap.size(); + } + + + @Override + public boolean isEmpty() { + return delegatedMap.isEmpty(); + } + + + @Override + public boolean containsKey(Object key) { + return delegatedMap.containsKey(key); + } - if (locked) - throw new IllegalStateException - (sm.getString("parameterMap.locked")); - return (super.remove(key)); + @Override + public boolean containsValue(Object value) { + return delegatedMap.containsValue(value); } + @Override + public V get(Object key) { + return delegatedMap.get(key); + } + + + /** + * {@inheritDoc} + * <p> + * Returns an <strong>unmodifiable</strong> {@link Set} view of the keys + * contained in this map if it is locked. + */ + @Override + public Set<K> keySet() { + if (locked) { + return unmodifiableDelegatedMap.keySet(); + } + + return delegatedMap.keySet(); + } + + + /** + * {@inheritDoc} + * <p> + * Returns an <strong>unmodifiable</strong> {@link Collection} view of the + * values contained in this map if it is locked. + */ + @Override + public Collection<V> values() { + if (locked) { + return unmodifiableDelegatedMap.values(); + } + + return delegatedMap.values(); + } + + + /** + * {@inheritDoc} + * <p> + * Returns an <strong>unmodifiable</strong> {@link Set} view of the mappings + * contained in this map if it is locked. + */ + @Override + public Set<java.util.Map.Entry<K, V>> entrySet() { + if (locked) { + return unmodifiableDelegatedMap.entrySet(); + } + + return delegatedMap.entrySet(); + } } Added: tomcat/trunk/test/org/apache/catalina/util/TestParameterMap.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/util/TestParameterMap.java?rev=1785667&view=auto ============================================================================== --- tomcat/trunk/test/org/apache/catalina/util/TestParameterMap.java (added) +++ tomcat/trunk/test/org/apache/catalina/util/TestParameterMap.java Mon Mar 6 15:28:47 2017 @@ -0,0 +1,310 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.catalina.util; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class TestParameterMap { + + private static final String[] TEST_PARAM_VALUES_1 = { "value1" }; + private static final String[] TEST_PARAM_VALUES_2 = { "value2" }; + private static final String[] TEST_PARAM_VALUES_2_UPDATED = { "value2-updated" }; + private static final String[] TEST_PARAM_VALUES_3 = { "value3" }; + private static final String[] TEST_PARAM_VALUES_REPLACED = { "replaced" }; + + private Map<String, String[]> paramMap; + + @Before + public void setUp() { + paramMap = new ParameterMap<>(); + + paramMap.put("param1", TEST_PARAM_VALUES_1); + paramMap.put("param2", TEST_PARAM_VALUES_2); + paramMap.put("param3", TEST_PARAM_VALUES_3); + + Assert.assertTrue(paramMap.containsKey("param1")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_1, paramMap.get("param1")); + Assert.assertTrue(paramMap.containsKey("param2")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_2, paramMap.get("param2")); + Assert.assertTrue(paramMap.containsKey("param3")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_3, paramMap.get("param3")); + + final Set<String> keySet = paramMap.keySet(); + Assert.assertTrue(keySet.contains("param1")); + Assert.assertTrue(keySet.contains("param2")); + Assert.assertTrue(keySet.contains("param3")); + + paramMap.put("param2", TEST_PARAM_VALUES_2_UPDATED); + paramMap.remove("param3"); + + Assert.assertTrue(paramMap.containsKey("param1")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_1, paramMap.get("param1")); + Assert.assertTrue(paramMap.containsKey("param2")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_2_UPDATED, paramMap.get("param2")); + Assert.assertFalse(paramMap.containsKey("param3")); + Assert.assertNull(paramMap.get("param3")); + + Assert.assertTrue(keySet.contains("param1")); + Assert.assertTrue(keySet.contains("param2")); + Assert.assertFalse(keySet.contains("param3")); + } + + @After + public void tearDown() { + Assert.assertTrue(paramMap.containsKey("param1")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_1, paramMap.get("param1")); + Assert.assertTrue(paramMap.containsKey("param2")); + Assert.assertArrayEquals(TEST_PARAM_VALUES_2_UPDATED, paramMap.get("param2")); + Assert.assertFalse(paramMap.containsKey("param3")); + Assert.assertNull(paramMap.get("param3")); + } + + @Test + public void testMapImmutabilityAfterLocked() { + ((ParameterMap<String, String[]>) paramMap).setLocked(true); + + try { + String[] updatedParamValues22 = new String[] { "value2-updated-2" }; + paramMap.put("param2", updatedParamValues22); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + String[] updatedParamValues22 = new String[] { "value2-updated-2" }; + paramMap.putIfAbsent("param22", updatedParamValues22); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + final Map<String, String[]> additionalParams = new HashMap<>(); + additionalParams.put("param4", new String[] { "value4" }); + paramMap.putAll(additionalParams); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + paramMap.merge("param2", new String[] { "value2-merged" }, (a, b) -> (b)); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + paramMap.remove("param2"); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + paramMap.remove("param2", TEST_PARAM_VALUES_2_UPDATED); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + paramMap.replace("param2", new String[] { "value2-replaced" }); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + paramMap.replace("param2", TEST_PARAM_VALUES_2_UPDATED, new String[] { "value2-replaced" }); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + + try { + paramMap.replaceAll((a, b) -> TEST_PARAM_VALUES_REPLACED); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + paramMap.clear(); + Assert.fail("ParameterMap is not locked."); + } catch (IllegalStateException expectedException) { + } + } + + @Test + public void testKeySetImmutabilityAfterLocked() { + ((ParameterMap<String, String[]>) paramMap).setLocked(true); + + final Set<String> keySet = paramMap.keySet(); + + try { + keySet.add("param4"); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + keySet.remove("param2"); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + keySet.removeIf((a) -> "param2".equals(a)); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + keySet.removeAll(Arrays.asList("param1", "param2")); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + keySet.retainAll(Collections.emptyList()); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + keySet.clear(); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + } + + @Test + public void testValuesImmutabilityAfterLocked() { + ((ParameterMap<String, String[]>) paramMap).setLocked(true); + + final Collection<String[]> valuesCol = paramMap.values(); + + try { + valuesCol.add(new String[] { "value4" }); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + List<String[]> list = new ArrayList<>(); + list.add(new String[] { "value4" }); + valuesCol.addAll(list); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + valuesCol.remove(TEST_PARAM_VALUES_1); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + valuesCol.removeIf((a) -> true); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + List<String[]> list = new ArrayList<>(); + list.add(TEST_PARAM_VALUES_1); + valuesCol.removeAll(list); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + valuesCol.retainAll(Collections.emptyList()); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + valuesCol.clear(); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + } + + @Test + public void testEntrySetImmutabilityAfterLocked() { + ((ParameterMap<String, String[]>) paramMap).setLocked(true); + + final Set<Map.Entry<String, String[]>> entrySet = paramMap.entrySet(); + + try { + final Map<String, String[]> anotherParamsMap = new HashMap<>(); + anotherParamsMap.put("param4", new String[] { "value4" }); + Map.Entry<String, String[]> anotherEntry = anotherParamsMap.entrySet().iterator().next(); + entrySet.add(anotherEntry); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + final Map<String, String[]> anotherParamsMap = new HashMap<>(); + anotherParamsMap.put("param4", new String[] { "value4" }); + anotherParamsMap.put("param5", new String[] { "value5" }); + entrySet.addAll(anotherParamsMap.entrySet()); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + final Map.Entry<String, String[]> entry = entrySet.iterator().next(); + entrySet.remove(entry); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + entrySet.removeIf((a) -> true); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + Set<Map.Entry<String, String[]>> anotherEntrySet = new HashSet<>(entrySet); + entrySet.removeAll(anotherEntrySet); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + entrySet.retainAll(Collections.emptySet()); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + + try { + entrySet.clear(); + Assert.fail("ParameterMap is not locked."); + } catch (UnsupportedOperationException expectedException) { + } + } +} \ No newline at end of file Modified: tomcat/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1785667&r1=1785666&r2=1785667&view=diff ============================================================================== --- tomcat/trunk/webapps/docs/changelog.xml (original) +++ tomcat/trunk/webapps/docs/changelog.xml Mon Mar 6 15:28:47 2017 @@ -157,6 +157,11 @@ that meant multiple attempts to read the same entry from a JAR in succession would fail for the second and subsequent attempts. (markt) </fix> + <fix> + <bug>60808</bug>: Ensure that the <code>Map</code> returned by + <code>ServletRequest.getParameterMap()</code> is fully immutable. Based + on a patch provided by woosan. (markt) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org