8345139: Fix bugs and inconsistencies in the Provider services map #22613

franferrax · 2024-12-06T19:36:09Z

Hi, this pull request implements the fixes for bugs and inconsistencies described in JDK-8345139.

New services map design

Here is the high-level hierarchy of the new services map design:

servicesMap (ServicesMap)
- Instances (fields)
  - services (Map<ServiceKey, Service>): unifies the previous serviceMap and legacyMap
  - legacySvcKeys (Set<ServiceKey>): set indicating which keys in services correspond to the Legacy API
  - serviceProps (Map<ServiceKey, String>): keeps track of the provider Hashtable entries that originated services entries ⁽¹⁾
  - serviceAttrProps (Map<ServiceKey, Map<UString, String>>): keeps track of the provider Hashtable entries that originated service attributes ⁽¹⁾
  - serviceSet (AtomicReference<Set<Service>>): part of a lock-free mechanism to implement a consistent version of the getServices() readers method
- Writers' methods (for providers registering services through the Current or the Legacy API)
  - boolean putService(Service svc)
  - boolean removeService(Service svc)
  - boolean putClassNameLegacy(ServiceKey key, String className, String propKey)
  - boolean putAliasLegacy(ServiceKey key, ServiceKey aliasKey, String propKey)
  - boolean putAttributeLegacy(ServiceKey key, String attrName, String attrValue, String propKey)
  - boolean removeLegacy(ServiceKey key, String className)
  - boolean removeAliasLegacy(ServiceKey key, ServiceKey aliasKey)
  - boolean removeAttributeLegacy(ServiceKey key, String attrName, String attrValue)
- Readers' methods (for services users and GetInstance APIs)
  - Set<Service> getServices()
  - Service getService(ServiceKey key)
- Other methods: default and copy constructors, void clear()

(1): to maintain the consistency between the provider's servicesMap and its Hashtable entries, even if Legacy API updates occur through properties with different casing, or aliases instead of main algorithms.

Testing

As part of our testing, we observed all the tests pass in the following categories:

jdk:tier1 (see GitHub Actions run)
jdk/com/sun/crypto
jdk/java/security
- Including the new jdk/java/security/Provider/ServicesConsistency.java

Additionally, we found no regressions with respect to this pull request baseline (bf0debc) in the following category:

jdk/sun/security
- Same results in both codebases
  
  Results
  
  Tests that passed 797
  
  Tests that failed 15
  
  Tests that had errors 2
  
  Tests that were not run 35
  
  Total 849

New `ServicesConsistency.java` test

The new ServicesConsistency test checks several of the cases described in the JBS issue, plus other similar variants. Here is a mapping between some of the test cases and their corresponding JBS issue section (using text fragments links), known to describe the tested problem.

Test case(s)	JBS section
`ServicesConsistency::testInvalidGetServicesRemoval`	JDK-8345139, section 1.1
`ServicesConsistency::testInvalidGetServiceRemoval`	JDK-8345139, section 1.2
`ServicesConsistency::testPutAllIsAtomic` `ServicesConsistency::testReplaceAllIsAtomic`	JDK-8345139, section 1.4
`ServicesConsistency::testInvalidCachedClass` `ServicesConsistency::testInvalidCachedHasKeyAttributes` `ServicesConsistency::testInvalidCachedSupportedKeyFormats`	JDK-8345139, section 1.5
`ServicesConsistency::testSerializationClassnameConsistency`	JDK-8345139, section 2.1
`ServicesConsistency::testCurrentAPIServicesOverride`	JDK-8345139, section 2.3
`ServicesConsistency::testLegacyAPIServicesOverride`	JDK-8345139, section 2.4
`ServicesConsistency::testLegacyAPIAliasCannotBeAlgorithm`	JDK-8345139, section 2.5
`ServicesConsistency::testInvalidServiceNotReturned`	JDK-8345139, section 3.1 (and JDK-8344361)
`ServicesConsistency::testComputeDoesNotThrowNPE` `ServicesConsistency::testMergeDoesNotThrowNPE`	JDK-8345139, section 3.2

This contribution is co-authored by @franferrax and @martinuy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8345139: Fix bugs and inconsistencies in the Provider services map (Bug - P4)

Contributors

Francisco Ferrari Bihurriet <fferrari@openjdk.org>
Martin Balao <mbalao@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22613/head:pull/22613
$ git checkout pull/22613

Update a local copy of the PR:
$ git checkout pull/22613
$ git pull https://git.openjdk.org/jdk.git pull/22613/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22613

View PR using the GUI difftool:
$ git pr show -t 22613

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22613.diff

Using Webrev

Link to Webrev Comment

Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com> Co-authored-by: Martin Balao <mbalao@redhat.com>

franferrax · 2024-12-06T19:37:02Z

/contributor add fferrari

franferrax · 2024-12-06T19:37:12Z

/contributor add mbalao

bridgekeeper · 2024-12-06T19:42:50Z

👋 Welcome back fferrari! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-12-06T19:43:26Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-12-06T19:44:06Z

@franferrax
Contributor Francisco Ferrari Bihurriet <fferrari@openjdk.org> successfully added.

openjdk · 2024-12-06T19:44:36Z

@franferrax
Contributor Martin Balao <mbalao@openjdk.org> successfully added.

openjdk · 2024-12-06T19:45:30Z

@franferrax The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-12-06T19:49:15Z

Webrevs

seanjmullan · 2024-12-11T17:23:27Z

/reviewers 2 Reviewer

seanjmullan · 2024-12-11T17:23:59Z

This is a large change so I think it should require at least 2 Reviewer approvals.

openjdk · 2024-12-11T17:24:14Z

@seanjmullan
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

ascarpino · 2024-12-11T19:08:52Z

src/java.base/share/classes/java/security/Provider.java

+        /*
+         * Enum to inform the result of an operation on the services map.
+         */
+        enum SvcOpResult {


Why use an enum here when a boolean is sufficient?

Just to add some more semantic value but we don't have a strong opinion, we can replace it with boolean if you want.

Yes, please do that.

Co-authored-by: Martin Balao Alonso <mbalao@redhat.com> Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com>

bridgekeeper · 2025-01-09T07:58:02Z

@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ascarpino

Could you explain the need for Current and Legacy interfaces? You have calls to doLegacy() for adding and removing entries, but I do not see why this is necessary since both APIs are in ServiceMapImpl.

ascarpino · 2025-01-21T23:54:02Z

src/java.base/share/classes/java/security/Provider.java

+        /*
+         * Enum to inform the result of an operation on the services map.
+         */
+        enum SvcOpResult {


Yes, please do that.

ascarpino · 2025-01-22T00:53:13Z

src/java.base/share/classes/java/security/Provider.java

+         */
+        private Service(Provider provider, ServiceKey algKey) {
+            assert algKey.algorithm.intern() == algKey.algorithm :
+                    "Algorithm should be interned.";


Why is intern() a requirement for this constructor?

Following the call stack this AssertionError is thrown with Provider.load() and Provider.putAll() at a minimum. This could change behavior and I think it should be removed.

These assertions are internal invariants that we want to sanity-check and document, specially for future code changes. They will never occur and do not depend on user API input. We went through all of them again to make sure that they are correct. For example, ServiceKey instances received in the referred Service constructor parameter must always have an internalized algorithm (the original algorithm string converted to uppercase). If you know of a call stack which may allow non-internalized algKey.algorithm, please let us know because it is a bug —we couldn't find any.

In the Security Manager days, the assumption was that Providers had enough privileges already to be trusted with internalizing Strings. On the contrary, code invoking getService was not necessarily trusted enough to internalize Strings used for queries. While this is not longer the case, we kept the distinction because Providers will use the Strings indeed and is a reduced set, whereas queries may be more open bounded.

ascarpino · 2025-01-23T01:19:45Z

src/java.base/share/classes/java/security/Provider.java

+                    "the algorithm.";
+            assert aliasKey.type.equals(type) : "Invalid alias key type.";
+            assert aliasKey.algorithm.intern() == aliasKey.algorithm :
+                    "Alias should be interned.";


All these asserts look like they leak into the public API. If something does not match your requirements, then log a detailed message using debug and do not add the entry.

Please have a look at my comment above.

openjdk · 2025-02-11T20:18:56Z

@franferrax this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8345139
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Co-authored-by: Martin Balao Alonso <mbalao@redhat.com> Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com>

Fix conflict caused by e20bd01: 1. Ignore the change, as this had already been identified and fixed (see JDK-8345139, section 3.1). 2. Remove the test, as it is already covered by ServicesConsistency::testInvalidServiceNotReturned. 3. Add the corresponding bug ID to ServicesConsistency. Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com> Co-authored-by: Martin Balao Alonso <mbalao@redhat.com>

martinuy · 2025-02-12T00:08:04Z

The reason for having Current and Legacy interfaces was only to internally group methods and stress the distinction. We have now removed it and proposed a flat hierarchy for the ServicesMap (without the interfaces and impl class).

franferrax · 2025-02-12T15:36:10Z

NOTE: as explained in the 35e2eae merge commit message, this PR now deletes InvalidServiceTest.java because that test is already covered by ServicesConsistency.java, since JDK-8344361 corresponds with JDK-8345139, section 3.1.

jdk/test/jdk/java/security/Provider/ServicesConsistency.java

Lines 835 to 839 in 35e2eae

    
           private static void testInvalidServiceNotReturned() throws Throwable { 
        
               p.put(aliasPropKeyL, algL); 
        
               assertServiceEqual(sT, algL, null); 
        
               assertServiceEqual(sT, aliasL, null); 
        
           }

Just in case, we made sure that the deleted InvalidServiceTest.java passes in this PR branch.

Constructors assign the fields in the same order.

valeriepeng · 2025-02-25T19:40:24Z

src/java.base/share/classes/java/security/Provider.java

+            classCache = null;
+            constructorCache = null;
+            hasKeyAttributes = null;
+            supportedFormats = null;
+            supportedClasses = null;


Are these necessary? The other constructor didn't set them.

valeriepeng · 2025-02-25T19:45:47Z

src/java.base/share/classes/java/security/Provider.java

+            } else {
+                attributes = new HashMap<>(svc.attributes);
+            }
+            registered = false;


I didn't see it's set to true in any of the constructors; also the default value is already false, why only explicitly set it to false here?

valeriepeng · 2025-02-25T23:49:41Z

src/java.base/share/classes/java/security/Provider.java

-        void removeAttribute(String type, String value) {
-            if (attributes.isEmpty()) {
-                return;
-            }
-            if (value == null) {
-                attributes.remove(new UString(type));
-            } else {
-                attributes.remove(new UString(type), value);
-            }
+        private void removeAttribute(String attrName, String attrValue) {
+            UString attrKey = new UString(attrName);
+            assert attributes.get(attrKey) == attrValue :
+                    "Attribute value expected to exist with the same identity.";
+            attributes.remove(attrKey, attrValue);
        }


Is the new impl assuming attrValue should never be null? Based on javadoc of Map.remove(Object key, Object value), the new impl removes the entry when the associated value is null vs the original impl removes the entry regardless of the associated value.

valeriepeng · 2025-02-26T00:07:43Z

src/java.base/share/classes/java/security/Provider.java

            this.className = className;
            if (aliases == null) {
                this.aliases = Collections.emptyList();
            } else {
                this.aliases = new ArrayList<>(aliases);
            }
+            aliasKeys = Collections.emptyMap();
            if (attributes == null) {
                this.attributes = Collections.emptyMap();
            } else {
                this.attributes = new HashMap<>();


Initialize with attributes.size() and load factor 1.0 if both are the same size?

valeriepeng · 2025-02-26T00:21:48Z

src/java.base/share/classes/java/security/Provider.java

+        // For services added to a ServicesMap, this is a map from alias service
+        // keys to alias string values. Empty map if no aliases. While map
+        // entries derive from the aliases field, keys are not repeated
+        // (case-insensitive comparison) and not equal to the algorithm. For
+        // services (still) not added to a ServicesMap, value is an empty map.


Could we re-write it to summarize the conditions for empty map? It could be easier to read/understand.
For example: empty map if no aliases or if this service is not yet added to a ServiceMap.
The part of case-insensitive comparision, it's due to the impl of ServiceKey, right? Maybe we can simply refer to that no need to describe it here.

valeriepeng · 2025-02-26T01:56:58Z

src/java.base/share/classes/java/security/Provider.java

+        // added to this map.
+        private final Map<ServiceKey, Map<UString, String>> serviceAttrProps;
+
+        ServicesMap() {


nit: add comment for this constructor?

valeriepeng · 2025-03-03T18:31:19Z

src/java.base/share/classes/java/security/Provider.java

@@ -635,7 +634,7 @@ public synchronized Object merge(Object key, Object value,

    // let javadoc show doc from superclass
    @Override
-    public Object get(Object key) {
+    public synchronized Object get(Object key) {


How about the getProperty(String) method on line 675? Add @Override tag and synchronized keyword there also? And the keySet() and values() methods on line 432 and 444 respectively? What is the criteria for synchronizing the method of the Provider class?

valeriepeng · 2025-03-03T23:24:29Z

src/java.base/share/classes/java/security/Provider.java

+         * values.
+         */
+        private record MappingInfo(Service svc, ServiceKey algKey,
+                Boolean isLegacy) {}


There is comment states that isLegacy may be null, but then I also see a few if-cond using isLegacy directly like its a boolean, wouldn't it lead to NPE if isLegacy is null?

valeriepeng · 2025-03-04T01:55:06Z

src/java.base/share/classes/java/security/Provider.java

+                    } else {
+                        // The service was added with the Current API. Overwrite
+                        // the alias entry on the services map without modifying
+                        // the service that is currently using it.


Is the "service" in the above line really means the provider service entry? If so, may be "associated with" is better than "using". Also there is no code under this comment block, where is the action of "overwrite the alias entry on the services map"?

valeriepeng · 2025-03-04T01:59:58Z

src/java.base/share/classes/java/security/Provider.java

+                return false;
+            }
+
+            if (mi.isLegacy) {


For legacy entry, there is no check on the legacyApiCall value, is this due to the call path from resolveKeyConflict method? However, should a legacy entry be removed by the removeService method? If not, then additional check may be needed?

valeriepeng · 2025-03-05T01:02:15Z

src/java.base/share/classes/java/security/Provider.java

+                String canonicalPropKey = propKey;
+                if (oldMi.svc != null) {
+                    // The service exists. Get its Properties map entry. Note:
+                    // Services added through an alias or an attribute may don't


"may don't"? Maybe you mean "may not" or simply "don't"

valeriepeng · 2025-03-05T01:40:55Z

src/java.base/share/classes/java/security/Provider.java

+        /*
+         * This method tries to find a service on the map (based on an
+         * algorithm or alias) and pass a copy of it to an update callback
+         * (copy-on-write). If the service found was added with the Current API,
+         * no update should be done. If a service was not found, a new instance
+         * may be created.
+         *
+         * The updated version of the service is put on the services map.
+         * Algorithm and alias based entries pointing to the old version of the
+         * service are overwritten.
+         */


Comment for this method should note that this method is only for the Legacy registration...

valeriepeng · 2025-03-05T07:06:45Z

src/java.base/share/classes/java/security/Provider.java

+        private boolean updateSvc(ServiceKey key,
+                ServiceUpdateCallback updateCb) {
+            Service newSvc;
+            MappingInfo oldMi = find(key);


oldMi could just simply be mi? There is no newMi in the same method anywhere.

valeriepeng · 2025-03-06T06:10:09Z

src/java.base/share/classes/java/security/Provider.java

+    /*
+     * Enum to determine if changes to the Properties map must be applied by the
+     * caller (UPDATE) or skipped (SKIP).
+     *
+     * If a change does not concern a ServicesMap, UPDATE is returned. An
+     * example of this is when adding, modifying or removing an entry that is
+     * not a service, alias or attribute.
+     *
+     * If the change concerns a ServicesMap, SKIP is returned. The change may
+     * have been applied internally or ignored due to an error. In the former
+     * case, Properties map entries are synchronized. In the latter, Properties
+     * map entries are not modified.
+     */


I am not too sure I get this comment block. Judging from the impl, this enum seems to be used to indicate whether the Properties map is updated. The part about ServiceMap is especially confusing. Is this enum for Properties map or ServicesMap? In addition, instead of stating "an entry that is not a service, alias or attribute.", can we just state the remaining types? Is "In the former case" refer to the UPDATE? In that paragraph. there is only one case. Lastly, there is only 2 values to this enum, can't this be replaced with a boolean?

valeriepeng · 2025-03-06T06:13:22Z

src/java.base/share/classes/java/security/Provider.java

+            } else if (value != null && oldValue instanceof String oldValueS &&
+                    opType == OPType.ADD) {


Which method is this else-block for? value is not null and not instanceof String and oldValue is instanceof String and opType is ADD?

valeriepeng · 2025-03-06T06:15:17Z

src/java.base/share/classes/java/security/Provider.java

+                // From the ServicesMap point of view, this could be equivalent
+                // to a removal. In any case, let the caller proceed with the
+                // Properties map update.
+                parseLegacy(servicesMap, ks, oldValueS, OPType.REMOVE);


no return here and falls through to the line 1512? Is this really intended?

valeriepeng · 2025-03-06T07:06:08Z

src/java.base/share/classes/java/security/Provider.java

-        }
-        return o;
+    private Object implRemove(Object key) {
+        Object oldValue = super.get(key);


oldValue isn't really old value, is it? Naming it oldValue and pass it to doLegacyOp() as value is very confusing.

8345139: Fix bugs and inconsistencies in the Provider services map

Loading
Loading status checks…

95364f8

Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com> Co-authored-by: Martin Balao <mbalao@redhat.com>

openjdk bot added the rfr label Dec 6, 2024

openjdk bot added the security label Dec 6, 2024

franferrax mentioned this pull request Dec 6, 2024

8315487: Security Providers Filter #15539

Open

4 tasks

ascarpino reviewed Dec 12, 2024

View reviewed changes

Copyright update.

Loading
Loading status checks…

bda8971

Co-authored-by: Martin Balao Alonso <mbalao@redhat.com> Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com>

ascarpino reviewed Jan 24, 2025

View reviewed changes

openjdk bot added the merge-conflict label Feb 11, 2025

martinuy and others added 2 commits February 11, 2025 18:10

ServicesMapImpl class and Current, Legacy interfaces removed.

Loading
Loading status checks…

ac45d7c

Co-authored-by: Martin Balao Alonso <mbalao@redhat.com> Co-authored-by: Francisco Ferrari Bihurriet <fferrari@redhat.com>

openjdk bot removed the merge-conflict label Feb 11, 2025

Clear ServicesMap fields in the declared order

Loading
Loading status checks…

3d0df51

Constructors assign the fields in the same order.

valeriepeng reviewed Mar 4, 2025

View reviewed changes

valeriepeng reviewed Mar 5, 2025

View reviewed changes

valeriepeng reviewed Mar 6, 2025

View reviewed changes

Results
Tests that passed	797
Tests that failed	15
Tests that had errors	2
Tests that were not run	35
Total	849

		} else if (value != null && oldValue instanceof String oldValueS &&
		opType == OPType.ADD) {

8345139: Fix bugs and inconsistencies in the Provider services map #22613

Are you sure you want to change the base?

8345139: Fix bugs and inconsistencies in the Provider services map #22613

Conversation

franferrax commented Dec 6, 2024 • edited by openjdk bot Loading

New services map design

Testing

New ServicesConsistency.java test

Progress

Issue

Contributors

Reviewing

franferrax commented Dec 6, 2024

franferrax commented Dec 6, 2024

bridgekeeper bot commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

mlbridge bot commented Dec 6, 2024 • edited Loading

Webrevs

seanjmullan commented Dec 11, 2024

seanjmullan commented Dec 11, 2024

openjdk bot commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgekeeper bot commented Jan 9, 2025

ascarpino left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 11, 2025

martinuy commented Feb 12, 2025

franferrax commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franferrax commented Dec 6, 2024 •

edited by openjdk bot

Loading

New `ServicesConsistency.java` test

mlbridge bot commented Dec 6, 2024 •

edited

Loading

ascarpino left a comment •

edited

Loading