RFR: 8343437: ClassDesc.of incorrectly permitting empty names [v2]

[Mandy Chung]

On Fri, 1 Nov 2024 19:24:05 GMT, Chen Liang <liach at openjdk.org> wrote:

>> In the patch for [JDK-8338544](https://bugs.openjdk.org/browse/JDK-8338544) #20665, the validation methods `validateBinaryClassName` and `validateInternalClassName` only checks if a separator char is the initial or final char, or if it immediately follows another chars.  This omitted the case of empty strings, and allowed creation of invalid ClassDesc with empty binary name, which is otherwise rejected by `ofDescriptor`.
>> 
>> To better check for the separator char, the tracking mechanism is updated to indicate a position where a separator char shouldn't appear, or where the name string should not terminate.  This is initially set to the initial position 0, and upon each time of encountering a separator, this is updated to the next char.
>> 
>> This logic is similar to the existing one in `skipOverFieldSignature`, which uses a boolean `legal` variable.  Both reject empty strings, leading and trailing separators, or consecutive separators.  The new logic, however, does not require repeated updates to the new `afterSeparator` variable upon scanning each character.
>> 
>> In addition, I noted the package name validation erroneously does not prohibit leading, trailing, or consecutive separator chars.  (Package names are derived from class or interface names, so the same restrictions shall apply)  This patch also makes package name validation reuse class or interface name validation in non-empty (unnamed package) cases, and added those cases to the test suite.
>
> Chen Liang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Comments to clarify, also align skipOverFieldSignature

src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 258:

> 256:         if (name.isEmpty())
> 257:             return name;
> 258:         return validateBinaryClassName(name);

Perhaps have a utility method to specify if empty name is allowed.   `validateBinaryClassName` and `validateInternalClassName` only differ in the separator charactor.   They can be refactored to call one single name validation method something like this:


    private static String validateName(String name, boolean internalName, boolean allowEmptyName) {
        if (!allowEmptyName && name.isEmpty())
              throw invalidClassName(name);
              
        // state variable for detection of illegal states, such as:
        // empty unqualified name, consecutive, leading, or trailing separators
        int afterSeparator = 0;
        int len = name.length();
        for (int i = 0; i < len; i++) {
            char ch = name.charAt(i);
            // reject ';' or '[' or other form's separator
            if (ch == ';' || ch == '[' || (internalName && ch == '.') || (!internalName && ch == '/'))
                throw invalidClassName(name);
            if ((internalName && ch == '/') || (!internalName && ch == '.')) {
                // illegal state when received separator indicates consecutive
                // or leading separators
                if (i == afterSeparator)
                    throw invalidClassName(name);
                afterSeparator = i + 1;
            }
        }
        // reject empty unqualified name or trailing separators
        if (len == afterSeparator)
            throw invalidClassName(name);
        return name;
    }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21830#discussion_r1826315242