8343437: ClassDesc.of incorrectly permitting empty names

[liach]

In the patch for 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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8343437: ClassDesc.of incorrectly permitting empty names (Bug - P3)

Reviewers

• Mandy Chung (@mlchung - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21830/head:pull/21830
$ git checkout pull/21830

Update a local copy of the PR:
$ git checkout pull/21830
$ git pull https://git.openjdk.org/jdk.git pull/21830/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21830

View PR using the GUI difftool:
$ git pr show -t 21830

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21830.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back liach! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@liach This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343437: ClassDesc.of incorrectly permitting empty names

Reviewed-by: mchung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 10 new commits pushed to the master branch:

• 29882bf: 8340311: JPackage app-image exe launches multiple exe's in JDK 22+
• 069bb79: 8342082: Remove unused BasicProgressBarUI.Animator.interval
• 00ec105: 8343412: Missing escapes for single quote marks in javac.properties
• 8c1cf8f: 8339128: Cannot resolve user specified tool properly after JDK-8338304
• 3c7082a: 8343419: Assertion failure in long vector unsigned min/max with -XX:+UseKNLSetting
• c82ad84: 8342183: Update tests to use stronger algorithms and keys
• 1eccdfc: 8343439: [JVMCI] Fix javadoc of Services.getSavedProperties
• ea110c3: 8343236: Use @APinote and @implSpec in j.util.Currency
• 5995786: 8343177: JFR: Remove critical section for thread id assignment
• 751a914: 8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@liach The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full - Incremental (469077d8)
• 02: Full - Incremental (dae377a4)
• 01: Full - Incremental (4325e94d)
• 00: Full (79d4dd18)

[mlchung]

Please describe where the bug was caused and also record the evaluation explaining the cause in the issue. Here is one part I understand for your reference.

One of the causes is that the refactoring done by JDK-8338544 converts the explicit check of the return value from ConstantUtils::skipOverFieldSignature called by ClassOrInterfaceDescImpl::ofValidated to assert. The refactoring was hard to review. So validateBinaryClassName only validates the binary name. Prior to JDK-8338544, the implementation expects more validations performed from ClassDesc::ofDescriptor.

I think it requires a thorough understanding of what being validated before JDK-8338544 to see if anything else is missing.

I am not sure what validation of package name is regressed. Please also explain.

[liach]

Before 8338544, the binary/internal name validation was partial. It missed a few validations performed by skipOverFieldSignature: no empty strings, leading or trailing or consecutive separator chars. In 8338544, the empty string validation was incorrectly missed.

PackageDesc in theory should not accept leading or trailing or consecutive separator chars, just like the names of classes and interfaces do not. Added validations for PackageDesc, so if we wish to create ClassDesc from a PackageDesc, we won't find invalid packages that cannot have classes or interfaces.

[mlchung]

Before 8338544, the binary/internal name validation was partial. It missed a few validations performed by skipOverFieldSignature: no empty strings, leading or trailing or consecutive separator chars. In 8338544, the empty string validation was incorrectly missed.

With your proposed change, all validations are done by validateBinaryClassName?

[liach]

With your proposed change, all validations are done by validateBinaryClassName?

Yes. Now all validations are done by that and validateInternalClassName, which uses the / separator instead of the . separator char. Meanwhile, the package name validation delegates to those two, and additionally permits the unnamed package represented by an empty string.

[mlchung]

Looks good with minor suggestion to rename the method name.

[liach]

/integrate

[openjdk]

Going to push as commit 1f7d524.
Since your change was applied there have been 25 commits pushed to the master branch:

• 895a7b6: 8342967: Lambda deduplication fails with non-metafactory BSMs and mismatched local variables names
• b41d713: 8343513: Forward declare Thread in mutexLocker.hpp
• 809030b: 8321500: javadoc rejects '@' in multi-line attribute value
• 7bca0af: 8343128: PassFailJFrame.java test result: Error. Bad action for script: build}
• f69b601: 8343188: Investigate ways to simplify MemorySegment::ofBuffer
• 7f131a9: 8343415: RISC-V: Increase maximum size of C2EntryBarrierStub by four
• 452a5fb: 8343507: Parallel: Fail if verify_complete finds incorrect states
• 7580199: 8343205: CompileBroker::possibly_add_compiler_threads excessively polls available memory
• df08a9e: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF
• e7f0bf1: 8343153: compiler/codecache/CheckLargePages.java fails on linux with huge pages configured but its number set to 0
• ... and 15 more: https://git.openjdk.org/jdk/compare/7e87c071b0fd832473f17ec0f579df40bea62950...master

Your commit was automatically rebased without conflicts.

[openjdk]

@liach Pushed as commit 1f7d524.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.