Re: RFR: 8340131: Refactor internal makeHiddenClassDefiner to take ClassOption ... instead of Set [v5]

[ExE Boss]

On Sun, 15 Sep 2024 12:59:21 GMT, Claes Redestad <redes...@openjdk.org> wrote:

>> Simple internal refactor to load a few classes less on startup. Arguably 
>> cleaner and avoids some iterator allocations.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Improve edge invariant checks

Commit 62a88c0368512e993df09c14d1b98b28b20e3280 introduced a TOCTOU issue 
caused by double‑reading of a user‑supplied array, the fix is to convert the 
resulting Set back to an array, or to eagerly convert the options to a flags 
`int` while checking for duplicates.

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 2140:

> 2138:             }
> 2139: 
> 2140:             return makeHiddenClassDefiner(bytes.clone(), false, 
> options).defineClassAsLookup(initialize);

Suggestion:

            // Disallow null and duplicate options
            var opts = Set.of(options);

            ensureDefineClassPermission();
            if (!hasFullPrivilegeAccess()) {
                throw new IllegalAccessException(this + " does not have full 
privilege access");
            }

            return makeHiddenClassDefiner(bytes.clone(), false, 
opts.toArray(ClassOption.NO_OPTIONS))
                       .defineClassAsLookup(initialize);

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 2230:

> 2228: 
> 2229:             return makeHiddenClassDefiner(bytes.clone(), false, options)
> 2230:                        .defineClassAsLookup(initialize, classData);

Suggestion:

            // Disallow null and duplicate options
            var opts = Set.of(options);

            ensureDefineClassPermission();
            if (!hasFullPrivilegeAccess()) {
                throw new IllegalAccessException(this + " does not have full 
privilege access");
            }

            return makeHiddenClassDefiner(bytes.clone(), false, 
opts.toArray(ClassOption.NO_OPTIONS))
                       .defineClassAsLookup(initialize, classData);

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

PR Review: https://git.openjdk.org/jdk/pull/21002#pullrequestreview-2305560599
PR Review Comment: https://git.openjdk.org/jdk/pull/21002#discussion_r1760276387
PR Review Comment: https://git.openjdk.org/jdk/pull/21002#discussion_r1760276870