Close leaked output streams (CodeQL java/output-resource-leak)

[vharseko]

Summary

Resolves 2 of the CodeQL java/output-resource-leak alerts by closing output
streams that could remain open when an exception is thrown before the explicit
close(). Behaviour is otherwise unchanged.

Location	Resource	Why it leaked	Fix
script/common Utils.copyURLToFile	input channel + FileOutputStream/output channel	both channels were opened outside the try; if new FileOutputStream(out) failed, the already-open input channel leaked	try-with-resources over all three
persistit/core CLI "dump"	ZipOutputStream over FileOutputStream	closed only on the normal path (stream.close() at the end); any exception while dumping buffer pools/volumes leaked the file	move stream.close() into a finally block

For the CLI "dump" fix a try/finally is used rather than try-with-resources:
the DataOutputStream (stream) and PrintWriter (writer) are views over
the same ZipOutputStream, and closing either one finalizes the archive and
closes the underlying FileOutputStream. So there is a single resource to
release — the outer DataOutputStream, closed in finally on every path. (The
bulk of the CLI diff is re-indentation of the existing body into the try
block.)

Notes on the remaining java/output-resource-leak alerts

The other alerts of this rule are handled separately:

• CLI "open" (OutputStreamWriter over the socket) — already fixed in the
  companion java/input-resource-leak PR, which wraps the Socket in
  try-with-resources.
• StreamSaver constructors, IOMeter.setLogFile and the audit
  StandardCsvWriter / SecureCsvWriter — the FileOutputStream is owned by
  the object and released through its own close() (the standard
  resource-owning pattern); dismissed as false positives.
• GroovyScript new PrintWriter(engine.getWriter(), true) — wraps the
  engine's borrowed writer and is handed to the script bindings as out;
  closing it would close the engine writer. Dismissed.
• The PrintWriter view inside CLI "dump" (java/output-resource-leak) and
  the pre-existing java/empty-zip-file-entry alert on the same .txt entry —
  dismissed as false positives; see the PR comment for the reasoning.

Testing

script/common and persistit/core compile.

[vharseko]

Re: the CodeQL comments on the dump command

I amended the CLI change (force-pushed) and dismissed the remaining findings as
false positives. Details:

Fix approach — why try/finally and not try-with-resources for dump.
The DataOutputStream (stream) and PrintWriter (writer) are views over
the same ZipOutputStream; closing either one finalizes the whole archive and
closes the underlying FileOutputStream. So there is only one resource to
manage. The FileOutputStream leak on the exception path is fixed by moving the
single stream.close() into a finally block, so the descriptor is released on
every path.

The first revision wrapped the ZipOutputStream in try-with-resources and
dropped stream.close(); that fixed the leak but made CodeQL flag the now-never-
closed DataOutputStream (a self-inflicted new alert). The try/finally form
closes the DataOutputStream explicitly, so that alert does not appear.

Dismissed as false positives:

• PrintWriter … not always closed (java/output-resource-leak) — the
  writer is a view over the ZipOutputStream, whose underlying FileOutputStream
  is closed on every path via the finally. It cannot be closed independently:
  doing so would finalize the ZIP archive early.
• Empty ZIP file entry created (java/empty-zip-file-entry, pre-existing
  alert #1737) — the .txt entry is always populated: writer.printf(...)
  writes @volumes/@trees/@bufferPools and writer.flush() pushes them to
  the ZipOutputStream before closeEntry(). The heuristic does not follow
  writes through the buffered PrintWriter wrapper.

[vharseko]

Addendum after the try/finally re-work: CodeQL re-scanned and re-anchored the
pre-existing java/empty-zip-file-entry finding onto both closeEntry()
calls (2014 @ the journal entry, 2015 @ the .txt entry). Both are the same
false positive and have been dismissed:

• the journal entry is written via stream (pool.dump(...) + stream.write(bb)
  • stream.flush()) before closeEntry();
• the .txt entry is written via writer.printf(...) + writer.flush() before
  closeEntry().

Neither entry is ever empty; the query just doesn't follow bytes through the
buffered DataOutputStream/PrintWriter wrappers. No behaviour change.

[vharseko]

Good catch — fixed in 63d26ef.

You're right that the try { … } finally { stream.close(); } form could mask the
original error: if the body (pool.dump, stream.write, zos.closeEntry, …)
throws and the stream.close() in finally also throws while finalizing the
ZIP, the close error would replace the original one.

Switched to your suggested form:

final ZipOutputStream zos = new ZipOutputStream(
        new BufferedOutputStream(new FileOutputStream(target), BUFFER_SIZE));
try (final DataOutputStream stream = new DataOutputStream(zos)) {
    ...
    writer.flush();
    zos.closeEntry();
}

There is a single resource to manage — closing the DataOutputStream cascades to
zos → BufferedOutputStream → FileOutputStream, and the PrintWriter is a
view over the same zos, so it needs no separate close. With try-with-resources a
failing close() is now added as a suppressed exception instead of masking the
original error, and the descriptor is still released on every path. Targets
Java 11 as noted.

Thanks also for confirming Utils.copyURLToFile — left unchanged.