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.

        Attachments

          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: