-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ensure open handles will not leak on errors #6326
fix: ensure open handles will not leak on errors #6326
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. |
core/src/main/java/org/owasp/dependencycheck/data/update/nvd/api/NvdApiProcessor.java
Outdated
Show resolved
Hide resolved
See suggested change. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 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
NvdApiProcessor
which ensures via auto-closing the streams will be closed anytimes.jsonFile
in 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-io
theorg.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