RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine

[Henry Jen]

On Fri, 11 Apr 2025 22:45:55 GMT, Chen Liang <liach at openjdk.org> wrote:

> When jdeps was migrated from old classfile to ClassFile API, the parsing semantic changed - error checks are now made lazily, and nested crashes from malformed signature or other problems is now latent, after a `ClassModel` instance is available. (The old error check existed only for constructing a `ClassModel`)
> 
> To address this issue, I have updated the way of iterating class files to be handler/consumer based like in the ClassFile API. This has the advantage that when one invocation of the handler fails of a `ClassFileError`, other invocations for other class files can proceed, and the exception handler has sufficient information to report a useful message indicating the source of error.
> 
> For the particular example of examining a proguard processed `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`:
> 
> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected character ; at position 59, expected an identifier: Lscala/collection/immutable/TreeMap$TreeMapBuilder<TA;TB;>.;: scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar)
> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected character ; at position 49, expected an identifier: Lscala/collection/parallel/mutable/ParArray<TT;>.;: scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar)
> 
> 
> Now, jdeps shows the bad class files. Inspection into the files reveal that proguard incorrectly deleted the simple class names with trailing `$`, for example, the original signature of the broken ParArray was `Lscala/collection/parallel/mutable/ParArray<TT;>.ParArrayIterator$;`, so the `ParArrayIterator$` part was incorrectly dropped by proguard.
> 
> Testing: langtools/tools/jdeps.

src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 105:

> 103: 
> 104:     protected void skipEntry(ClassFileError ex, String fileName) {
> 105:         skippedEntries.add(String.format("%s: %s", ex.toString(), fileName));

The second parameter is not always a straightforward filename, consider to rename it, perhaps `entryPath`?

src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 237:

> 235:                               skipEntry(ex, e.toString());
> 236:                           } catch (IOException ex) {
> 237:                               throw new UncheckedIOException(ex);

Just to point out this was leading to ClassFileError in the old implementation, new implementation will relay the IOException, which I think is proper.

src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 339:

> 337:                         });
> 338:             } catch (UncheckedIOException ex) {
> 339:                 throw ex.getCause();

IOException used to skip entry with message and continue, the new behavior would change. I am not certain this is behavior compatible.
I doubt IOException would be recoverable on different entry, but it might in rare cases?

src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java line 176:

> 174:         FutureTask<Set<Location>> task = new FutureTask<>(() -> {
> 175:             Set<Location> targets = new HashSet<>();
> 176:             archive.reader().processClassFiles(cf -> {

I prefer the naming convention with forEach for operation to iterate through rather than have a `process` on a reader. Perhaps named like `forEachClassFile`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049319406
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049411874
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049425413
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049378312