Max attributes per element limit only loosely enforced

[jhaeyaert]

Hello,

Given the following snippet, XmlStreamReader should throw an exception indicating that the number of attributes for the given element has been reached.

XMLInputFactory factory = new WstxInputFactory();
        factory.setProperty(WstxInputProperties.P_MAX_ATTRIBUTES_PER_ELEMENT, Integer.valueOf(2));

        Reader reader = new StringReader("<element att1=\"foo1\" att2=\"foo2\" att3=\"foo3\" />");
        XMLStreamReader xmlreader = factory.createXMLStreamReader(reader);
        try {
            while (xmlreader.hasNext()) {
                xmlreader.next();
            }
            fail("Should have failed");
        } catch (XMLStreamException ex) {
            assertTrue(ex.getMessage().contains("Attribute limit (2)"));
        }
        reader.close();

Looking at AttributeCollector class (line 801), it seems that attribute count is compared to attributes array whose size is increased by 50% (so length of the array become greater than real number of attributes in the array).

            if (mAttrCount >= mAttributes.length) {
                if ((mAttrCount + mNsCount) >= mMaxAttributesPerElement) {
                    throw new XMLStreamException("Attribute limit ("+mMaxAttributesPerElement+") exceeded");
                }
                mAttributes = (Attribute[]) DataUtil.growArrayBy50Pct(mAttributes);
            }

Do I missed something or the "max attribute condition" should be checked apart ?

[cowtowncoder]

Maximum attribute limit is designed to guard against various overflow / Denial-of-Service attacks, so checking may be approximate -- general contract would be to allow at least specified number of attributes, but not necessarily fail immediately. This would likely be the case for low limits like 2.
This is why many other checks -- especially ones affecting tight loops, like decoding character data -- only check for limit at boundary conditions so checks themselves do not add measurable overhead.

In case of attributes there may be room for improvement, so that buffer length would not grow beyond limit and thereby check could be (more) accurate -- so that, for example, with limit of, say, 1000 attributes, it would not grow to 1200 (assuming 50% increase went to 800, allowed, then to 1200) but would be hit at 1000.

It has been a while since I looked at this particular check so I can't say for sure but perhaps it could even work for small limits like 2.

So I think this would be an improvement, to have more sensitive check for exceeding maximum number.

[jhaeyaert]

Thanks for your explanations.
Indeed, the following improvements could be applied without side effect or performance degradation:

1. Initialize / Resize the array being careful not to exceed the mMaxAttributesPerElement
2. Check the number of attributes doesn't exceed the limit before resizing the array.

if (mAttrCount >= mAttributes.length) {
    if ((mAttrCount + mNsCount) >= mMaxAttributesPerElement) {
        throw new XMLStreamException("Attribute limit ("+mMaxAttributesPerElement+") exceeded");
    }
    mAttributes = (Attribute[]) DataUtil.growArrayBy50Pct(mAttributes);
}

With

if ((mAttrCount + mNsCount) >= mMaxAttributesPerElement) {
    throw new XMLStreamException("Attribute limit ("+mMaxAttributesPerElement+") exceeded");
}
    
if (mAttrCount >= mAttributes.length) {
    mAttributes = (Attribute[]) DataUtil.growArrayBy50Pct(mAttributes);
}

[cowtowncoder]

yes, that sounds reasonable. I hope to find some time soon (perhaps tonight) to look into this.

[cowtowncoder]

Fixed, added tests. Due to combination of namespace declarations and attributes it is possible that limit is still not strictly enforced in all possible cases (for some namespace declarations, some attributes).
But should be closer at least.