Fix potential NPEs in IpcClient and resource leak in DependencyGraphParser - #1945
Conversation
…arser - IpcClient.getJarPath(): handle null ClassLoader (bootstrap classes) and null resource lookup with explicit null checks - IpcClient.receive(): capture volatile `input` field in local variable before use to prevent NPE from concurrent close() - IpcClient.getAddress(): capture volatile `socket` field in local variable and add null check to prevent NPE during concurrent close() - DependencyGraphParser.parseMultiResource(): wrap BufferedReader in try-with-resources to prevent stream leak on exception Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR hardens IPC client code against null races and missing resources, and fixes a resource leak in the dependency graph parser.
Changes:
- Add try-with-resources in
DependencyGraphParser.parseMultiResource()to ensure streams are closed. - Prevent NPEs in
IpcClientby adding null-safe resource lookup ingetJarPath()and local copies of volatile fields inreceive()/getAddress().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| maven-resolver-test-util/src/main/java/org/eclipse/aether/internal/test/util/DependencyGraphParser.java | Ensures readers/streams are closed reliably during multi-resource parsing. |
| maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java | Adds null handling for classpath resource lookup and avoids racy volatile field access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ClassLoader classLoader = clazz.getClassLoader(); | ||
| if (classLoader == null) { | ||
| classLoader = ClassLoader.getSystemClassLoader(); | ||
| } | ||
| URL resource = classLoader.getResource(className); | ||
| if (resource == null) { | ||
| throw new IllegalStateException("Unable to find resource for class " + clazz.getName()); | ||
| } |
There was a problem hiding this comment.
Good catch. Switched to clazz.getResource("/" + className) which handles bootstrap-loaded classes natively — Class.getResource() internally delegates to ClassLoader.getSystemResource() when the classloader is null, so no explicit fallback is needed. Fixed in c2464e0.
| if (resource == null) { | ||
| throw new IllegalStateException("Unable to find resource for class " + clazz.getName()); | ||
| } |
There was a problem hiding this comment.
IllegalStateException is consistent with the two existing throws in the same method (lines 280 and 285) for other unrecoverable URL format conditions. The caller at line 176 is inside a catch (Exception e) { throw new RuntimeException("Unable to create and connect to lock server", e) } block (line 257), so the exception is always caught and wrapped with context — nothing is bypassed.
Address Copilot review: replace explicit ClassLoader fallback with Class.getResource() which natively handles bootstrap-loaded classes by delegating to ClassLoader.getSystemResource() internally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
getClassLoader()returnsnullfor bootstrap classes, andgetResource()returnsnullwhen the resource is not found. Either causes an NPE on the chained.toString()call. Added explicit null checks with fallback to system classloader and a clear error message.inputfield is read and used without a local copy. A concurrentclose()call can null the field between the read and the method invocation, causing an NPE. Fixed by capturing the volatile read in a local variable.socketfield can be nulled by a concurrentclose()call. The resulting NPE is not anIOExceptionand propagates uncaught. Fixed by capturing in a local variable with a null check.BufferedReader(and its underlyingInputStream) is never closed. Ifparse(reader)throws, the stream leaks. Wrapped in try-with-resources, consistent with the existingparse(URL)method at line 174.Test plan
mvn compilepasses for both modified modulesmvn testpasses formaven-resolver-named-locks-ipcmvn testpasses formaven-resolver-test-util🤖 Generated with Claude Code