DO NOT REPLY [Bug 44991] New: Concurrent invocation of KeyInfo. getX509Certificate() occasionally fails

https://issues.apache.org/bugzilla/show_bug.cgi?id=44991

           Summary: Concurrent invocation of KeyInfo.getX509Certificate()
                    occasionally fails
           Product: Security
           Version: unspecified
          Platform: PC
        OS/Version: All
            Status: NEW
          Severity: critical
          Priority: P2
         Component: Signature
        AssignedTo: security-dev@...
        ReportedBy: giedrius.noreikis@...


When executed concurrently in several threads,
org.apache.xml.security.keys.KeyInfo.getX509Certificate() occasionally returns
null.


The log entries made from the failing thread are:
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.KeyInfo
getX509CertificateFromInternalResolvers
Start getX509CertificateFromInternalResolvers() with 0 resolvers
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.KeyInfo
getX509Certificate
I couldn't find a X509Certificate using the per-KeyInfo key resolvers
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.KeyInfo
getX509CertificateFromStaticResolvers
Start getX509CertificateFromStaticResolvers() with 7 resolvers
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.keyresolver.implementations.X509SKIResolver
engineLookupResolveX509Certificate
Can I resolve X509Data?
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.keyresolver.implementations.X509SKIResolver
engineLookupResolveX509Certificate
I can't
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.keyresolver.implementations.X509SubjectNameResolver
engineLookupResolveX509Certificate
Can I resolve X509Data?
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.keyresolver.implementations.X509SubjectNameResolver
engineLookupResolveX509Certificate
I can't
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.keyresolver.implementations.X509IssuerSerialResolver
engineLookupResolveX509Certificate
Can I resolve X509Data?
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.utils.ElementProxy
<init>
setElement("X509Data", "http://www.w3.org/2000/09/xmldsig#")
--------------------------------------------------
2008-05-13T20:21:50
org.apache.xml.security.keys.KeyInfo
getX509Certificate
I couldn't find a X509Certificate using the system-wide key resolvers
--------------------------------------------------

Possible cause:
KeyInfo.getX509CertificateFromStaticResolvers() operates on
org.apache.xml.security.keys.keyresolver.KeyResolver class: it iterates through
all KeyResolver items, trying to applyCurrentResolver(), and, in case of
success, calls KeyResolver.hit().
When getX509CertificateFromStaticResolvers() in Thread-1 founds a "good"
resolver at iteration, say, i=5, and calls hit(), that resolver is moved at the
beginning of the static KeyResolver._resolverVector list. If Thread-2 at the
same time executes getX509CertificateFromStaticResolvers() at iteration, say,
i=3, it will never see that resolver.

Possible fix:
With the present design, it seems, KeyResolver can not support item() and hit()
methods together, since hit() changes the order of the _resolverVector items.
Either hit() should be removed or a copy of _resolverVector should be made
before accessing it's elements.


--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/bugzilla/show_bug.cgi?id=44991





--- Comment #1 from Raul Benito <raul-info@...>  2008-05-18 10:12:45 PST ---
Thanks for the report. I don't found it, but you are right. I forgot to use the
pointer of the old list to make the RCU working.
With this change it should work and shouldn't be more concurrency problems.
This change is already commited to CVS head.

Thanks again

Index: src/org/apache/xml/security/keys/keyresolver/KeyResolver.java
===================================================================
--- src/org/apache/xml/security/keys/keyresolver/KeyResolver.java      
(revision 657575)
+++ src/org/apache/xml/security/keys/keyresolver/KeyResolver.java      
(working copy)
@@ -123,9 +123,11 @@
            Element element, String BaseURI, StorageResolver storage)
               throws KeyResolverException {

-      for (int i = 0; i < KeyResolver._resolverVector.size(); i++) {
+         // use the old vector to not be hit by updates
+         List resolverVector = KeyResolver._resolverVector;
+      for (int i = 0; i < resolverVector.size(); i++) {
                  KeyResolver resolver=
-            (KeyResolver) KeyResolver._resolverVector.get(i);
+            (KeyResolver) resolverVector.get(i);

                  if (resolver==null) {
             Object exArgs[] = {
@@ -165,10 +167,11 @@
    public static final PublicKey getPublicKey(
            Element element, String BaseURI, StorageResolver storage)
               throws KeyResolverException {
-
-      for (int i = 0; i < KeyResolver._resolverVector.size(); i++) {
+        
+         List resolverVector = KeyResolver._resolverVector;
+      for (int i = 0; i < resolverVector.size(); i++) {
                  KeyResolver resolver=
-            (KeyResolver) KeyResolver._resolverVector.get(i);
+            (KeyResolver) resolverVector.get(i);

                  if (resolver==null) {
             Object exArgs[] = {
@@ -186,7 +189,7 @@
          if (cert!=null) {
                 if (i!=0) {
                 //update resolver.                      
-                        List
resolverVector=(List)((ArrayList)_resolverVector).clone();                      
+                      
resolverVector=(List)((ArrayList)_resolverVector).clone();                      
                                 Object ob=resolverVector.remove(i);
                                 resolverVector.add(0,ob);
                                 _resolverVector=resolverVector;


--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/bugzilla/show_bug.cgi?id=44991





--- Comment #2 from Raul Benito <raul-info@...>  2008-05-18 12:16:27 PST ---
Sorry the patch only change half of the problem. I'm working in a solution but
it should be in the like of returning a list for iteration.
I will create a single thread unit testing.
Thanks again


--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/bugzilla/show_bug.cgi?id=44991





--- Comment #3 from Giedrius Noreikis <giedrius.noreikis@...>  2008-05-18 13:15:15 PST ---
Created an attachment (id=21978)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=21978)
KeyResolverVector

Yes, copying just a *reference* to the *same* object changes nothing - returned
list must be a copy of the *object*, which will not be affected by the
subsequent updates.

Reproducing this bug is not easy (nevertheless, it appears in our production
from time to time). I create 100 threads, each sleeps random time before
proceeding, and repeat the procedure 100 times. Almost always it fails at some
iteration.

In case you are interested in my solution, I attach the files (I couldn't wait
any longer, 'cause I had to fix this bug in our system). I decided to introduce
a new class - KeyResolverVector - to encapsulate the shared resource and the
synchronized methods for accessing it. Of course, KeyResolver and KeyInfo had
to be adjusted accordingly.

Regards,
Giedrius


--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.