RFR: 8345139: Fix bugs and inconsistencies in the Provider services map [v2]

[Martin Balao]

On Wed, 22 Jan 2025 00:53:13 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Copyright update.
>>   
>>   Co-authored-by: Martin Balao Alonso <mbalao at redhat.com>
>>   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
>
> src/java.base/share/classes/java/security/Provider.java line 2223:
> 
>> 2221:         private Service(Provider provider, ServiceKey algKey) {
>> 2222:             assert algKey.algorithm.intern() == algKey.algorithm :
>> 2223:                     "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.

> src/java.base/share/classes/java/security/Provider.java line 2279:
> 
>> 2277:             assert aliasKey.type.equals(type) : "Invalid alias key type.";
>> 2278:             assert aliasKey.algorithm.intern() == aliasKey.algorithm :
>> 2279:                     "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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1951784544
PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1951784974