Elliotte reported bugs

[cstamas]

Affected version

1.0.0

These were all pulled out by throwing the code at an LLM. This issue lists multiple independent bugs. If anyone wants to work on any of these, note it here and open a new issue just for that bug. Be careful that some of these are likely false reports, though closing those is also useful work.

Bug description

Bug Report: maven-executor

HIGH SEVERITY

1. toBuilder().argument(String) throws UnsupportedOperationException
   ExecutorRequest.java:150-165, 249-255
   toBuilder() passes arguments() (an unmodifiable list from Impl)
   into the Builder constructor,
   which stores the reference directly. When argument(String) is
   called, it invokes
   this.arguments.add(...) on the unmodifiable list, crashing at runtime.

2. MAVEN_ARGS leaks to child process in ForkedMavenExecutor
   ForkedMavenExecutor.java:109, 143, 149-153
   Line 109 reads System.getenv("MAVEN_ARGS") from the parent process.
   Line 143 does
   env.remove("MAVEN_ARGS"), but env is a local HashMap containing
   only request-specified
   variables, not the parent process environment. Line 152 calls
   pb.environment().putAll(env)
   but MAVEN_ARGS from the parent is never removed from the inherited
   environment, so the
   child process also inherits it.

3. extractMTPSingleArgument throws StringIndexOutOfBoundsException
   MavenToolProvider.java (java9):146
   When an argument like -MEX-mavenHome is passed without = and a value,
   argument.substring(key.length() + 1) throws StringIndexOutOfBoundsException.

4. Null values from extractMTPMapArgument produce string "null" in output
   MavenToolProvider.java (java9):165-169 ->
   ForkedMavenExecutor.java:127 -> DockerExeExecutor.java:91
   When -MEX-env=MY_VAR is passed without a =value, the map stores
   null. This propagates to
   "MY_VAR=null" in DockerExeExecutor and "-Dkey=null" in ForkedMavenExecutor.

5. ToolboxExecutorTool.doExecute() uses request.stdOut() (an
   OutputStream reference) instead
   of result.stdOutString() in error messages
   ToolboxExecutorTool.java:144-146
   The error message prints request.stdOut().orElse(null) which
   returns the OutputStream object
   (e.g., java.io.ByteArrayOutputStream@123456), not the captured
   output. The actual stdout/stderr
   content is in result.stdOutString()/result.stdErrString() but is never read.

MEDIUM SEVERITY

6. stdIn InputStream is never closed (resource leak)
   ProcessBuilderExecutorSupport.java:149-158
   In the stdinPump thread, the stdIn InputStream is read from but
   never closed. The
   try-with-resources only closes in (the process's OutputStream). The
   Javadoc says
   "The stream is closed once tool execution is finished" but this
   does not happen.

7. fs4WithDollar test uses MAVEN3_HOME instead of MAVEN4_HOME (copy-paste error)
   MavenExecutorTestSupport.java:329
   The @disabled test fs4WithDollar (intended for Maven 4) references
   Environment.MAVEN3_HOME
   at line 329.

8. System.setProperties(null) creates thread-safety window
   EmbeddedMavenExecutor.java:383
   prepareProperties() calls System.setProperties(null) followed by
   properties.putAll(System.getProperties()). Between these two calls,
   any concurrent thread
   accessing System.getProperties() may get null or empty properties.

9. ExecutorHelperImpl.close() does not catch RuntimeException from
   executor close()
   ExecutorHelperImpl.java:88-109
   Only ExecutorException is caught when closing executors. If an
   executor's close() throws
   a RuntimeException (e.g., NullPointerException), it propagates
   immediately and prevents
   subsequent executors from being closed.

10. UncheckedIOException not wrapped as ExecutorException in
    TestContainersExecutor
    TestContainersExecutor.java:161-166
    detectUid() wraps IOException in UncheckedIOException (a
    RuntimeException). The execute()
    method's catch blocks only catch IOException. The
    UncheckedIOException propagates uncaught.

11. NPE if resource files are missing in TestProjects.createSimpleProject
    TestProjects.java:37, 40
    TestProjects.class.getResourceAsStream(...) can return null, and
    Files.copy(InputStream, Path)
    will throw NullPointerException instead of a clear "resource not
    found" message.

LOW SEVERITY

12. extractMTPSingleArgument removes ALL matching arguments, not just the first
    MavenToolProvider.java (java9):145
    allArguments.removeIf(s -> s.equals(argument)) removes every
    occurrence, silently
    discarding duplicates.

13. Builder.stdOut()/stdErr() don't clear the other stream
    ExecutorRequest.java:324-334
    Setting stdOut(...) sets grabOutputAsString = false but does not
    clear this.stdErr,
    and vice versa.

14. Temp directory deletion may fail in ForkedMavenExecutor.mavenVersion()
    ForkedMavenExecutor.java:74, 93
    Files.createTempDirectory(...) is deleted in finally via
    Files.deleteIfExists(cwd), but
    if the Maven process creates files inside this directory, the
    delete fails with
    DirectoryNotEmptyException.

15. nullInputStream.read(byte[], int, int) violates InputStream contract
    IOTools.java:83-89
    The read(byte[], int, int) contract requires throwing
    IndexOutOfBoundsException if off is
    negative, len is negative, or off + len > b.length. No bounds
    validation is performed.

16. Deprecated ToolboxExecutorTool(Executor) hardcodes 0.15.8 vs current 0.15.14
    ToolboxExecutorTool.java:53-56
    The deprecated constructor uses an old cemented version while the
    POM defines
    version.toolbox as 0.15.14.

17. @DisabledOnOs comment vs annotation mismatch in Docker provider tests
    TestContainersExecutorTest.java:38, DockerExeExecutorTest.java:38
    Comment says "not supported on Windows" but annotation also
    disables on OS.MAC.

[cstamas]

@elharo just to not forget this