Skip to content

Close streams that were leaked when the work on them failed - #614

Open
JakeHuneau wants to merge 1 commit into
OpenAS2:masterfrom
JakeHuneau:fix/stream-descriptor-leaks
Open

JakeHuneau wants to merge 1 commit into
OpenAS2:masterfrom
JakeHuneau:fix/stream-descriptor-leaks

Conversation

@JakeHuneau

Copy link
Copy Markdown
Contributor

Four places open a stream, do some work and close the stream afterwards, so the close is skipped whenever the work throws.

The one that matters

X509CertificateFactory.load(String, char[]) and save(String, char[]):

FileInputStream fIn = new FileInputStream(filename);
load(fIn, password);      // throws OpenAS2Exception, not IOException
fIn.close();              // skipped
} catch (IOException ioe) {

load(InputStream, char[]) declares throws OpenAS2Exception and wraps everything it catches into WrappedException, so a wrong password or a corrupt file propagates past the catch (IOException) and the close never runs.

PKCS12CertificateFactory reloads the keystore on a schedule, five minutes apart by default, so a keystore that consistently fails to load leaks a descriptor every refresh, roughly 288 a day, until the process can no longer open files. The failure is silent until then.

save has the identical shape and leaks an output stream held open on the keystore itself.

The other three

  • DirectoryResenderModule leaks on a resend file that fails to deserialise. Bounded, since a bad file is quarantined to the error directory, so it is one descriptor per bad file rather than per poll.
  • NetModule leaks in the handler that stores an invalid message, if the write fails.
  • XMLCommandRegistry.refresh never closes its stream on any path.

All four now use try-with-resources.

Testing

182 tests pass, 2 new. The test counts the process's open descriptors through /proc across repeated failed and successful loads, and skips where /proc is unavailable so it does not fail the Windows matrix.

Reverting just the keystore load to the hand written close makes it fail with "40 failed loads should not each hold a descriptor open, went from 125 to 165", which is one leaked descriptor per attempt exactly as described.

Four places opened a stream, did some work and closed the stream afterwards,
so the close was skipped whenever the work threw.

The keystore load and save are the ones that matter. Both call through to an
overload that throws OpenAS2Exception, which the catch around the hand written
close only covers IOException, so every failure leaked the descriptor. The
keystore is reloaded on a schedule, five minutes apart by default, so a
keystore that consistently fails to load leaked a descriptor per refresh until
the process could no longer open files. The save leaks an output stream held
open on the keystore itself.

The resender leaked on a resend file that failed to deserialise, the network
module leaked in the handler that stores an invalid message, and the command
registry never closed its stream on any path at all.

All four now use try-with-resources so the stream is closed whichever way the
work ends.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant