Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Mon, 10 Feb 2020 13:27:27 GMT, Ambarish Rapte wrote: >> Memory allocated in initDecompressor() and decompressIndirect() is not freed >> in error case. >> In error case, >> 1. Allocated memory should be freed. >> 2. Appropriate de-initialization jpeg library calls should be added. >> >> Verified that, >> 1. All unit and systems tests pass on three platforms, and >> 2. Memory consumption with and without fix is similar by comparing memory >> before and after showing 10 jpeg images for 100 times. > > The pull request has been updated with a new target base due to a merge or a > rebase. Looks good to me. I paid special attention to null pointer dereferencing when cleaning up resources, but I couldn't spot a trace that could cause this. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/54
Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 26 Feb 2020 22:17:31 GMT, Kevin Rushforth wrote: >> The pull request has been updated with a new target base due to a merge or a >> rebase. > > Marked as reviewed by kcr (Lead). @johanvos , @arajkumar Could you please take a re-look at the updated PR. - PR: https://git.openjdk.java.net/jfx/pull/54
Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Mon, 10 Feb 2020 13:27:27 GMT, Ambarish Rapte wrote: >> Memory allocated in initDecompressor() and decompressIndirect() is not freed >> in error case. >> In error case, >> 1. Allocated memory should be freed. >> 2. Appropriate de-initialization jpeg library calls should be added. >> >> Verified that, >> 1. All unit and systems tests pass on three platforms, and >> 2. Memory consumption with and without fix is similar by comparing memory >> before and after showing 10 jpeg images for 100 times. > > The pull request has been updated with a new target base due to a merge or a > rebase. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/54
Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Fri, 7 Feb 2020 10:06:59 GMT, Ambarish Rapte wrote: >> In general, this makes sense. I need to look into more detail that the >> additional calls for freeing resources (in case of errors) don't cause e.g. >> segmentation violations and lead to a crash -- which would be worse than >> throwing an Exception. >> I expect memory consumption to be similar before and after this patch if you >> don't run into errors, but did you check memory consumption before/after >> this patch in case of errors? > > The PR was outdated. It is now merged with latest master. > Please take a look. @johanvos , @arajkumar Could you please take a re-look at the updated PRs. - PR: https://git.openjdk.java.net/jfx/pull/54
Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
> Memory allocated in initDecompressor() and decompressIndirect() is not freed > in error case. > In error case, > 1. Allocated memory should be freed. > 2. Appropriate de-initialization jpeg library calls should be added. > > Verified that, > 1. All unit and systems tests pass on three platforms, and > 2. Memory consumption with and without fix is similar by comparing memory > before and after showing 10 jpeg images for 100 times. The pull request has been updated with a new target base due to a merge or a rebase. - Commits: - b91e4236: Merge with master - 8a48080a: correcting use of scanline_ptr - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case Changes: https://git.openjdk.java.net/jfx/pull/54/files Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.02 Stats: 59 lines in 1 file changed: 33 ins; 15 del; 11 mod Patch: https://git.openjdk.java.net/jfx/pull/54.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54 PR: https://git.openjdk.java.net/jfx/pull/54
Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 21:19:08 GMT, Johan Vos wrote: >> The pull request has been updated with a new target base due to a merge or a >> rebase. > > In general, this makes sense. I need to look into more detail that the > additional calls for freeing resources (in case of errors) don't cause e.g. > segmentation violations and lead to a crash -- which would be worse than > throwing an Exception. > I expect memory consumption to be similar before and after this patch if you > don't run into errors, but did you check memory consumption before/after this > patch in case of errors? The PR was outdated. It is now merged with latest master. Please take a look. - PR: https://git.openjdk.java.net/jfx/pull/54
