fix: ensure open handles will not leak on errors

[knalli]

Fixes Issue

As mentioned in #6275 (comment), I found possible hidden memory leaks in case of broken inputs.

Background: Before #6275, the I/O handling (managing streams, file handling) itself was handled in NvdApiProcessor (IMHO the right place) which also handled the auto-closing on errors. After the change, the stream creation and parsing have moved both into the source implementation classes. If the readItem() methods fail (unexpected), the source constructor will not be succeeded, which means no instance at all, which means no auto-close with try-catch and therefore the created input streams will not be closed anymore.

Also the close runs several closes in sequential order ignoring any exceptions happening there.

Also the read file will not be deleted, however that may be fine.

Description of Change

• The creations of input streams is back in NvdApiProcessor which ensures via auto-closing the streams will be closed anytimes.
• Because I did not managed to find a reference of deleting the original file jsonFile in the previous version, I skipped this for now. Maybe reconsider adding again if this is required.
• The closing implemented in the reader classes (e.e. JsonArrayCveItemSource) may suffer from leaking when a sequential list of closables breaks in the line. Even if this is not the case today, it can change anytimes depending on the external tooling (here Jackson). I have decided to use from commons-io:commons-io the org.apache.commons.io.IOUtils#closeQuietly(java.io.Closeable...). If the errors should propagate, then use #close(java.io.Closeable...)

Have test cases been added to cover the new functionality?

no

[philippn]

Hi @knalli ,

did you test drive this code? I would expect it to throw errors at runtime because the InputStreams are closed by the try-with-resources blocks inside of buildItemSource. They need to be open during the lifetime of the CveItemSource, in order for the streaming read to work properly. I would suggest to remove the try-with-resources there.

Kind regards,
Philipp

[knalli]

Oh, good catch. Thank you having a look. After applying my original purpose, I thought it would be good to move everything into a builder method.

Well, I have issues running the tests (missed this in the PR to mention). Although I have a JDK 1.8 it blocks because of an invalid byte code level. Thought I can wait for the test run here, unfortunately, it did not run.

[jeremylong]

See suggested change.

[knalli]

I think I have managed to run the tests locally, without changing any default profile:

[WARNING] Tests run: 473, Failures: 0, Errors: 0, Skipped: 13

However that is a run in which I disabled the NvdApiProcessor#call() with an explicit exception effectively. Looks like there is a test case missing. In case I did not overseen something, an additional, regular unit test should be fine?

[knalli]

I'm adding an additional commit for a simple unit test which should cover the technical issues. I don't know where to get a "real" file input, so the current processValidStructure tests only for a "valid json" [].

[jeremylong]

LGTM