On Sat, 2 Nov 2024 00:16:41 GMT, Chen Liang <li...@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: > > Reuse the check implementation Looks good with minor suggestion to rename the method name. src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 191: > 189: * @throws NullPointerException if name is {@code null} > 190: */ > 191: private static String validatePathBasedName(String name, boolean > usesSlash, boolean allowsEmpty) { Suggestion: private static String validatePackageOrClassName(String name, boolean internalName, boolean allowEmpty) { ------------- Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21830#pullrequestreview-2411032617 PR Review Comment: https://git.openjdk.org/jdk/pull/21830#discussion_r1826400726
