Uploaded image for project: 'OpenSAML 2 - Java'
  1. OpenSAML 2 - Java
  2. JOST-174

ChainingMetadataProvider calls clear() on unmodifiable list

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5.2
    • Component/s: None
    • Labels:
      None

      Description

      in method setProviders, code is:

      if (newProviders == null || newProviders.isEmpty())

      { providers.clear(); return; }

      but the default object for providers is:

      providers = Collections.EMPTY_LIST;

      This was changed between 2.3.1 and 2.5.1. In 2.3.1, providers was an empty array list.

      I believe the fix to this is to replace providers.clear() with providers = Collections.EMPTY_LIST.

        Attachments

          Expenses

            Activity

            Hide
            philvarner@idp.protectnetwork.org philvarner@idp.protectnetwork.org added a comment -

            This bug breaks the Spring Security SAML Extension library.

            Show
            philvarner@idp.protectnetwork.org philvarner@idp.protectnetwork.org added a comment - This bug breaks the Spring Security SAML Extension library.
            Hide
            philvarner@idp.protectnetwork.org philvarner@idp.protectnetwork.org added a comment -

            After looking at this a bit more, a better fix is to get rid of the special case and early return for empty list entirely, and make the method:

            public void setProviders(List<MetadataProvider> newProviders) throws MetadataProviderException {
            Lock writeLock = providerLock.writeLock();
            writeLock.lock();

            try {
            ArrayList<MetadataProvider> checkedProviders = new ArrayList<MetadataProvider>();
            if (newProviders != null) {
            for (MetadataProvider provider : newProviders)

            { doAddMetadataProvider(provider, checkedProviders); }

            }
            providers = Collections.unmodifiableList(checkedProviders);
            } finally

            { writeLock.unlock(); }

            }

            Show
            philvarner@idp.protectnetwork.org philvarner@idp.protectnetwork.org added a comment - After looking at this a bit more, a better fix is to get rid of the special case and early return for empty list entirely, and make the method: public void setProviders(List<MetadataProvider> newProviders) throws MetadataProviderException { Lock writeLock = providerLock.writeLock(); writeLock.lock(); try { ArrayList<MetadataProvider> checkedProviders = new ArrayList<MetadataProvider>(); if (newProviders != null) { for (MetadataProvider provider : newProviders) { doAddMetadataProvider(provider, checkedProviders); } } providers = Collections.unmodifiableList(checkedProviders); } finally { writeLock.unlock(); } }
            Hide
            lajoie@shibboleth.net Chad La Joie added a comment -

            fixed in rev 1558

            Show
            lajoie@shibboleth.net Chad La Joie added a comment - fixed in rev 1558

              People

              • Assignee:
                lajoie@shibboleth.net Chad La Joie
                Reporter:
                philvarner@idp.protectnetwork.org philvarner@idp.protectnetwork.org
              • Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: