elharo opened a new issue, #1944:
URL: https://github.com/apache/maven-resolver/issues/1944
### Affected version
HEAD
### Bug description
# Bug Analysis: maven-resolver
Generated by code analysis of `/Users/elharo/maven-resolver`
(v2.0.21-SNAPSHOT).
---
## HIGH Severity
### 1. NPE — `getClassLoader().getResource()` returns null
**File:**
`maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java:265`
```java
String url = clazz.getClassLoader().getResource(className).toString();
```
`ClassLoader.getResource()` returns `null` when the resource is not found,
and `getClassLoader()` returns `null` for classes loaded by the bootstrap class
loader. Either condition causes an NPE on `.toString()`. The method
`getJarPath()` is called during client initialization to build the classpath
for forking a child process.
### 2. NPE — `receive()` reads volatile `input` without synchronization
**File:**
`maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java:288-311`
```java
void receive() {
try {
while (true) {
int id = input.readInt(); // NPE here if input null
```
`input` is a volatile field (line 86). `receive()` reads it without holding
the `IpcClient` monitor. Meanwhile, `close(Throwable)` (line 351,
`synchronized`) sets `input = null` (line 360). Between the volatile read at
line 291 and the `input.readInt()` call, another thread can call `close()`,
nulling `input`. The same pattern applies to `send()` at line 316 with `output`.
### 3. NPE — `getAddress()` can NPE on `socket.getLocalAddress()`
**File:**
`maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java:447-453`
```java
private String getAddress() {
try {
return SocketFamily.toString(socket.getLocalAddress());
} catch (IOException e) {
return "[not bound]";
}
}
```
`socket` is volatile (line 84) and set to `null` in `close()` (line 359). If
`close()` runs concurrently with `toString()` (which calls `getAddress()`),
`socket` is null and `socket.getLocalAddress()` throws NPE, which is **not** an
`IOException` and is thus uncaught.
### 4. Resource leak — `BufferedReader` never closed in
`parseMultiResource()`
**File:**
`maven-resolver-test-util/src/main/java/org/eclipse/aether/internal/test/util/DependencyGraphParser.java:161`
```java
BufferedReader reader = new BufferedReader(new
InputStreamReader(res.openStream(), StandardCharsets.UTF_8));
```
The `BufferedReader` (and its underlying `InputStream`) is **never closed**.
If `parse(reader)` throws an IOException, the stream leaks. Compare with the
correctly-implemented `parse(URL)` method at line 174-188 which uses
try-finally.
---
## MEDIUM Severity
### 5. Catching `Throwable` in non-framework code
**File:**
`maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/PutTaskRequestContent.java:246,
298`
```java
} catch (Throwable t) {
lockedSetTerminal(Content.Chunk.from(t, true));
}
```
Catches `OutOfMemoryError`, `StackOverflowError`, `InternalError`, etc.
These should propagate. Wrapping them as terminal chunks converts fatal JVM
errors into regular failure conditions, making them invisible to diagnostics.
### 6. Production error logging to `System.out`
**File:**
`maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcServer.java:196`
```java
private static void error(String msg, Throwable t) {
System.out.println("[ipc] [error] " + msg);
t.printStackTrace(System.out);
}
```
All errors in the IPC server go to stdout instead of a proper logging
framework (SLF4J). In a production Maven build, these messages are invisible or
interleaved with build output. Same issue for `debug()` (line 184) and `info()`
(line 190) in the same file.
### 7. Listener RuntimeException silently swallowed by default
**Files:**
-
`maven-resolver-util/src/main/java/org/eclipse/aether/util/listener/ChainedRepositoryListener.java:117`
-
`maven-resolver-util/src/main/java/org/eclipse/aether/util/listener/ChainedTransferListener.java:118`
```java
@SuppressWarnings("EmptyMethod")
protected void handleError(...) {
// default just swallows errors
}
```
All `RuntimeException`s thrown by any chained listener (NPEs,
`IndexOutOfBoundsException`, etc.) are silently swallowed unless the user
subclasses and overrides `handleError`. This makes listener bugs invisible.
### 8. Volatile fields with inconsistent synchronization
**File:**
`maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java:78-87`
Fields `initialized`, `socket`, `output`, `input`, `receiver` are `volatile`
but their compound use (read + method call) lacks synchronization. `send()`
reads `output` into a local variable at line 316 without holding the monitor,
then synchronizes on the local reference at line 323, but `close()` nulls
`output` under `synchronized(this)` at line 361. `receive()` has the same
pattern with `input`.
### 9. SyncContext double-close risk
**Files:**
-
`maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:189-190,
393-397, 430-432`
-
`maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java:142-143,
302-373`
Two `SyncContext` instances (`shared` and `exclusive`) are created in
try-with-resources. Inside `resolve()`, `current` starts as `shared`, and when
switching to `exclusive` (line 393-396), `current.close()` is called, then
`current = exclusive`. The outer try-with-resources also closes both on exit
(line 202 for try-with-resources, and line 431 for the inner
`current.close()`). While `SyncContext.close()` is documented as idempotent,
any implementation that violates this contract would cause issues.
### 10. InterruptedException causes task to be silently dropped
**File:**
`maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/SmartExecutor.java:177-179`
```java
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
```
In `Limited.submit(Runnable)` (line 155), if `semaphore.acquire()` throws
`InterruptedException`, the interrupt flag is restored but the caller's task is
never executed. The method simply returns. The `submit(Callable)` variant (line
183-213) correctly handles this by returning a failed `CompletableFuture`. The
`Runnable` overload should do something analogous (e.g., run the task inline or
throw).
---
## LOW Severity
### 11. Minor resource leak in `parseLiteral()`
**File:**
`maven-resolver-test-util/src/main/java/org/eclipse/aether/internal/test/util/DependencyGraphParser.java:134-138`
```java
BufferedReader reader = new BufferedReader(new
StringReader(dependencyGraph));
DependencyNode node = parse(reader);
reader.close(); // never reached if parse() throws
```
While `StringReader.close()` is a no-op, the pattern is inconsistent with
the rest of the codebase and represents a latent bug if the implementation
changes.
### 12. Empty catch swallows exceptions in `tryUnlock()`
**File:**
`maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcNamedLock.java:108-115`
```java
private void tryUnlock(String contextId) {
try {
client.unlock(contextId);
} catch (Exception e) {
// Best-effort cleanup
}
}
```
All exceptions from `unlock()` during timeout cleanup are silently
discarded. While documented as intentional, this can mask real IO/connectivity
issues.
### 13. Non-thread-safe access in synchronized `Results` class
**File:**
`maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java:571-598`
`addException()` and `addCycle()` are `synchronized` on `Results`, but they
call `result.getExceptions()` and `result.getCycles()` on a `CollectResult`
that may not be thread-safe. If `CollectResult`'s internal collections are not
synchronized or safely published, concurrent modifications may not be visible.
### 14. `putIfAbsent()` fast-path race
**File:**
`maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/ConcurrentWeakCache.java:124-146`
```java
V existing = get(key); // fast path
if (existing != null) {
return existing;
}
```
The fast-path `get()` uses a ThreadLocal `LookupKey`. Between `get()`
returning null and `merge()` at line 135, another thread can insert a new
value. The merge correctly handles this, but if `get()` returns a non-null
reference to a key that gets GC'd immediately after, we return a stale
reference. This is a correct-by-accident pattern — the merge would fix it, but
we return early.
### 15. Pre-Java-7 stream handling in test utilities
**Files:**
-
`maven-resolver-test-util/src/main/java/org/eclipse/aether/internal/test/util/TestFileUtils.java:172-325`
-
`maven-resolver-test-util/src/main/java/org/eclipse/aether/internal/test/util/TestFileProcessor.java:63-151`
Methods manually close streams in try-catch-finally blocks instead of using
try-with-resources. If an explicit `close()` call throws before the finally
block, open resources are leaked. Functionally correct in happy-path but
fragile.
### 16. `printStackTrace()` used instead of logging in tests
Multiple test files use `e.printStackTrace()` instead of proper assertions
or test framework logging, making test failures harder to diagnose. Affected
files include:
-
`maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultArtifactResolverTest.java:740,
752, 805, 874, 975, 987`
- `maven-resolver-demos/src/main/java/.../Booter.java:111`
-
`maven-resolver-api/src/test/java/org/eclipse/aether/DefaultSessionDataTest.java:118`
-
`maven-resolver-api/src/test/java/org/eclipse/aether/DefaultRepositoryCacheTest.java:82`
---
## Summary
| Severity | Count | Key Areas |
|----------|-------|-----------|
| HIGH | 4 | IpcClient (3 NPEs), DependencyGraphParser (resource
leak) |
| MEDIUM | 6 | PutTaskRequestContent (Throwable catch), IpcServer
(stdout logging), ChainedListener (silent swallow), IpcClient (sync),
SyncContext double-close, SmartExecutor (dropped task) |
| LOW | 6 | Minor resource leak, swallowed exception, thread safety
in Results, ConcurrentWeakCache race, old stream handling, printStackTrace |
The **IPC named-locks module** (`maven-resolver-named-locks-ipc`) accounts
for the majority of HIGH-severity issues — it has multiple NPEs from
unsynchronized volatile field access.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
