fix: ensure open handles will not leak on errors#6326
fix: ensure open handles will not leak on errors#6326jeremylong merged 2 commits intodependency-check:mainfrom
Conversation
|
Hi @knalli , did you test drive this code? I would expect it to throw errors at runtime because the Kind regards, |
|
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. |
|
See suggested change. |
knalli
force-pushed
the
feature/handle_leaks_on_error
branch
from
c940c49 to
e578b48
Compare
|
I think I have managed to run the tests locally, without changing any default profile:
However that is a run in which I disabled the |
|
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 |
3 participants
Fixes Issue
As mentioned in jeremylong/DependencyCheck#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 thereadItem()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
NvdApiProcessorwhich ensures via auto-closing the streams will be closed anytimes.jsonFilein the previous version, I skipped this for now. Maybe reconsider adding again if this is required.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 fromcommons-io:commons-iotheorg.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