This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 13b34e11cd228f29878a1b67ffb9ebfe4c57c0c4 Author: opencode <[email protected]> AuthorDate: Mon Oct 19 17:11:12 2026 +0200 Build UniqueAttributesImpl.setAttributes() from the incoming set setAttributes() validated the incoming attributes against the store they were about to replace and never reset the duplicate tracking set. A repeated import on a page directive was merged into an attribute of the old store that was immediately discarded, or threw an ArrayIndexOutOfBoundsException when that store held no import yet; a duplicate tolerated by the page directive rules was copied into the store verbatim; and names from the replaced attributes stayed behind in the tracking set, where they were later reported as false duplicates. Build the replacement store and the tracking set from the incoming attributes alone, so the duplicate rules act on each other's results, and install them only once the whole set validates, leaving the instance untouched when a duplicate is rejected. Generalise handleDuplicate() to take the store it acts on; the delegating overload preserves the behaviour of the addAttribute(), setAttribute() and setQName() paths. Extend TestUniqueAttributesImpl with the replacement cases. Found by code review. --- .../apache/jasper/util/UniqueAttributesImpl.java | 32 +++-- .../jasper/util/TestUniqueAttributesImpl.java | 132 +++++++++++++++++++++ 2 files changed, 157 insertions(+), 7 deletions(-) diff --git a/java/org/apache/jasper/util/UniqueAttributesImpl.java b/java/org/apache/jasper/util/UniqueAttributesImpl.java index e36268417d..174a94a819 100644 --- a/java/org/apache/jasper/util/UniqueAttributesImpl.java +++ b/java/org/apache/jasper/util/UniqueAttributesImpl.java @@ -59,12 +59,26 @@ public class UniqueAttributesImpl extends AttributesImpl { @Override public void setAttributes(Attributes atts) { + // Build the replacement store separately so that the incoming + // attributes are validated and merged against each other rather than + // against the attributes being replaced, and so that a rejected + // duplicate leaves this instance untouched + AttributesImpl merged = new AttributesImpl(); + Set<String> names = new HashSet<>(); for (int i = 0; i < atts.getLength(); i++) { - if (!qNames.add(atts.getQName(i))) { - handleDuplicate(atts.getQName(i), atts.getValue(i)); + String qName = atts.getQName(i); + String value = atts.getValue(i); + if (names.add(qName)) { + merged.addAttribute(atts.getURI(i), atts.getLocalName(i), qName, + atts.getType(i), value); + } else { + handleDuplicate(qName, value, merged); } } - super.setAttributes(atts); + + super.setAttributes(merged); + qNames.clear(); + qNames.addAll(names); } @Override @@ -113,12 +127,16 @@ public class UniqueAttributesImpl extends AttributesImpl { } private void handleDuplicate(String qName, String value) { + handleDuplicate(qName, value, this); + } + + private void handleDuplicate(String qName, String value, AttributesImpl store) { if (pageDirective) { if (IMPORT.equalsIgnoreCase(qName)) { // Always merge imports - int i = super.getIndex(IMPORT); - String v = super.getValue(i); - super.setValue(i, v + "," + value); + int i = store.getIndex(IMPORT); + String v = store.getValue(i); + store.setValue(i, v + "," + value); return; } else if (PAGE_ENCODING.equalsIgnoreCase(qName)) { // Page encoding can only occur once per file so a second @@ -126,7 +144,7 @@ public class UniqueAttributesImpl extends AttributesImpl { } else { // Other attributes can be repeated if and only if the values // are identical - String v = super.getValue(qName); + String v = store.getValue(qName); if (v.equals(value)) { return; } diff --git a/test/org/apache/jasper/util/TestUniqueAttributesImpl.java b/test/org/apache/jasper/util/TestUniqueAttributesImpl.java index 68b2810e2c..ea46f43d1b 100644 --- a/test/org/apache/jasper/util/TestUniqueAttributesImpl.java +++ b/test/org/apache/jasper/util/TestUniqueAttributesImpl.java @@ -19,6 +19,9 @@ package org.apache.jasper.util; import org.junit.Assert; import org.junit.Test; +import org.xml.sax.Attributes; +import org.xml.sax.helpers.AttributesImpl; + public class TestUniqueAttributesImpl { @@ -29,6 +32,15 @@ public class TestUniqueAttributesImpl { return attrs; } + private static Attributes attributes(String... nameValuePairs) { + AttributesImpl atts = new AttributesImpl(); + for (int i = 0; i < nameValuePairs.length; i += 2) { + atts.addAttribute("", nameValuePairs[i], nameValuePairs[i], "CDATA", + nameValuePairs[i + 1]); + } + return atts; + } + /* * Renaming an attribute to a new name must register that name, so a @@ -117,4 +129,124 @@ public class TestUniqueAttributesImpl { // expected } } + + + /* + * Replacing the attributes must reset the duplicate tracking, so names + * from the replaced attributes are no longer reported as duplicates and + * names of the new attributes are. + */ + @Test + public void testSetAttributesResetsTracking() throws Exception { + UniqueAttributesImpl attrs = createAttributes(); + + attrs.setAttributes(attributes("x", "3", "y", "4")); + + Assert.assertEquals(2, attrs.getLength()); + Assert.assertEquals("x", attrs.getQName(0)); + + // The replaced attributes' names must no longer be tracked + attrs.addAttribute("", "a", "a", "CDATA", "5"); + + // The new attributes' names must be tracked + try { + attrs.addAttribute("", "x", "x", "CDATA", "6"); + Assert.fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // expected + } + } + + + /* + * The incoming set must be validated against itself, not against the + * attributes being replaced, so a name held by a replaced attribute may + * be reused with any value. + */ + @Test + public void testSetAttributesReplacesHeldName() throws Exception { + UniqueAttributesImpl attrs = createAttributes(); + + attrs.setAttributes(attributes("a", "9")); + + Assert.assertEquals(1, attrs.getLength()); + Assert.assertEquals("9", attrs.getValue("a")); + } + + + /* + * Replacing the attributes with a set containing a rejected duplicate + * must leave both the attributes and the tracking untouched. + */ + @Test + public void testSetAttributesRejectedDuplicateLeavesStateUntouched() throws Exception { + UniqueAttributesImpl attrs = createAttributes(); + + try { + attrs.setAttributes(attributes("m", "3", "m", "4")); + Assert.fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // expected + } + + Assert.assertEquals(2, attrs.getLength()); + Assert.assertEquals("a", attrs.getQName(0)); + + // The rejected set's names must not have been left behind in the + // tracking either + attrs.addAttribute("", "m", "m", "CDATA", "5"); + Assert.assertEquals(3, attrs.getLength()); + + try { + attrs.addAttribute("", "a", "a", "CDATA", "6"); + Assert.fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // expected + } + } + + + /* + * On a page directive, repeated import attributes in the incoming set + * must be merged into a single attribute. + */ + @Test + public void testSetAttributesMergesImports() throws Exception { + UniqueAttributesImpl attrs = new UniqueAttributesImpl(true); + + attrs.setAttributes(attributes("import", "a", "import", "b", + "contentType", "text/html")); + + Assert.assertEquals(2, attrs.getLength()); + Assert.assertEquals("a,b", attrs.getValue("import")); + + // A later import still merges + attrs.addAttribute("", "import", "import", "CDATA", "c"); + Assert.assertEquals("a,b,c", attrs.getValue("import")); + } + + + /* + * On a page directive, an attribute in the incoming set repeated with an + * identical value must be tolerated without storing the duplicate, while + * a repeated pageEncoding must be rejected even with an identical value. + */ + @Test + public void testSetAttributesPageDirectiveRules() throws Exception { + UniqueAttributesImpl attrs = new UniqueAttributesImpl(true); + + attrs.setAttributes(attributes("foo", "1", "foo", "1")); + Assert.assertEquals(1, attrs.getLength()); + + try { + attrs.setAttributes(attributes("pageEncoding", "UTF-8", + "pageEncoding", "UTF-8")); + Assert.fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // expected + } + // The rejected set left the previous attributes in place + Assert.assertEquals(1, attrs.getLength()); + Assert.assertEquals("1", attrs.getValue("foo")); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
