On Fri, 1 Nov 2024 19:24:05 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: > > 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
