We are having problems with an org.apache.xml.security.signature.XMLSignature instance creation in a multi-threaded environment: sometimes an ArrayIndexOutOfBoundsException is thrown: java.lang.ArrayIndexOutOfBoundsException: 38 at java.util.ArrayList.add(Unknown Source) at org.apache.xml.security.keys.KeyInfo.<init>(Unknown Source) at org.apache.xml.security.signature.XMLSignature.<init>(Unknown Source) ... The XMLSignature constructor being used is: public XMLSignature(Element element, String BaseURI) Looking further at the sources I found out that: 1. The KeyInfo constructor being invoked by the XMLSignature constructor must be public KeyInfo(Element element, String BaseURI) (XMLSignature:297). 2. The exact line producing the exception must be _storageResolvers.add(null) (KeyInfo:123). 3. Upon the creation, the _storageResolvers variable of *each* KeyInfo instance holds a reference to the *single* static nullList variable (KeyInfo:1067). Thus, adding null to that list effectively modifies the single shared ArrayList instance, while concurrent access and structural modifications of an ArrayList instance are not allowed. 4. The entire _storageResolvers.add(null) statement seems to be useless and probably could be simply removed. 5. IMHO, such a strange invention :) as that nullList should be evaluated and probably removed as well.
I'd like Raul to comment on this one, since I think this is part of his performance improvement changes. It seems that if we just removed line 123 of KeyInfo.java, it would fix the problem: _storageResolvers.add(null); If there is a simple, low-risk fix then I am open to adding this to 1.4.2. (In reply to comment #0) > We are having problems with an org.apache.xml.security.signature.XMLSignature > instance creation in a multi-threaded environment: sometimes an > ArrayIndexOutOfBoundsException is thrown: > > java.lang.ArrayIndexOutOfBoundsException: 38 > at java.util.ArrayList.add(Unknown Source) > at org.apache.xml.security.keys.KeyInfo.<init>(Unknown Source) > at org.apache.xml.security.signature.XMLSignature.<init>(Unknown Source) > ... > > The XMLSignature constructor being used is: > public XMLSignature(Element element, String BaseURI) > > Looking further at the sources I found out that: > 1. The KeyInfo constructor being invoked by the XMLSignature constructor must > be public KeyInfo(Element element, String BaseURI) (XMLSignature:297). > 2. The exact line producing the exception must be _storageResolvers.add(null) > (KeyInfo:123). > 3. Upon the creation, the _storageResolvers variable of *each* KeyInfo instance > holds a reference to the *single* static nullList variable (KeyInfo:1067). > Thus, adding null to that list effectively modifies the single shared ArrayList > instance, while concurrent access and structural modifications of an ArrayList > instance are not allowed. > 4. The entire _storageResolvers.add(null) statement seems to be useless and > probably could be simply removed. > 5. IMHO, such a strange invention :) as that nullList should be evaluated and > probably removed as well. >
According to my tests, removing this line fixes the problem. If more radical changes are undesirable due to a higher risk, to prevent such bugs in future I would propose at least making that nullList final and unmodifiable: static final List nullList; static { List list = new ArrayList(); list.add(null); nullList = Collections.unmodifiableList(list); } BTW, currently I'm working on a project using xmlsec library, and I have to fix such issues anyway (just made own build).. So, in case you need help, I could contribute more, by fixing bugs I've found, for example. (In reply to comment #1) > I'd like Raul to comment on this one, since I think this is part of his > performance improvement changes. It seems that if we just removed line 123 > of KeyInfo.java, it would fix the problem: > _storageResolvers.add(null); > If there is a simple, low-risk fix then I am open to adding this to 1.4.2. > (In reply to comment #0) > > We are having problems with an org.apache.xml.security.signature.XMLSignature > > instance creation in a multi-threaded environment: sometimes an > > ArrayIndexOutOfBoundsException is thrown: > > > > java.lang.ArrayIndexOutOfBoundsException: 38 > > at java.util.ArrayList.add(Unknown Source) > > at org.apache.xml.security.keys.KeyInfo.<init>(Unknown Source) > > at org.apache.xml.security.signature.XMLSignature.<init>(Unknown Source) > > ... > > > > The XMLSignature constructor being used is: > > public XMLSignature(Element element, String BaseURI) > > > > Looking further at the sources I found out that: > > 1. The KeyInfo constructor being invoked by the XMLSignature constructor must > > be public KeyInfo(Element element, String BaseURI) (XMLSignature:297). > > 2. The exact line producing the exception must be _storageResolvers.add(null) > > (KeyInfo:123). > > 3. Upon the creation, the _storageResolvers variable of *each* KeyInfo instance > > holds a reference to the *single* static nullList variable (KeyInfo:1067). > > Thus, adding null to that list effectively modifies the single shared ArrayList > > instance, while concurrent access and structural modifications of an ArrayList > > instance are not allowed. > > 4. The entire _storageResolvers.add(null) statement seems to be useless and > > probably could be simply removed. > > 5. IMHO, such a strange invention :) as that nullList should be evaluated and > > probably removed as well. > >
Good Analysis. I don't know why I forgot to remove add(null) in the contractor that was the purpose of nullList, don't create and add a null to a list that 98% percent of the time will just have a null. And not to change the rest of the code that depend on the null, as we don't have enough (any?) testcases that check that behavior. I have just add all the lines(including the unmodifiableList) even after checking that the only add left checks before if the list is the null one. It looks like a good way to detect the next error. Thanks for detecting and for the report. Regards