On Wed, 22 Jan 2025 00:53:13 GMT, Anthony Scarpino <ascarp...@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 <mba...@redhat.com>
>> Co-authored-by: Francisco Ferrari Bihurriet <fferr...@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
