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

      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.

        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: