Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Mon, 2 Dec 2019 12:14:21 GMT, Ambarish Rapte wrote:
> On Thu, 28 Nov 2019 11:12:42 GMT, Arunprasad Rajkumar
> wrote:
>
>> On Wed, 27 Nov 2019 11:58:18 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.
>>>
>>>
>>>
>>> Commits:
>>> - 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.00
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8212034
>>> Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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
>>
>> modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1625:
>>
>>> 1624:
>>> 1625: JSAMPROW scanline_ptr = (JSAMPROW) malloc(bytes_per_row * sizeof
>>> (JSAMPLE));
>>> 1626: if (scanline_ptr == NULL) {
>>
>> You can remove quite a few calls to `free` if you move the memory allocation
>> for `scanline_ptr` just [before it's
>> usage](https://github.com/openjdk/jfx/blob/7af932b7f5215949776ec79fb2a5484c521b21a1/modules/javafx.graphics/src/main/native-iio/jpegloader.c#L1690).
>> Also free it as soon as you are done with it.
>
> PR is updated according to this comment, please have a look.
> 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.
The native code throws two Exceptions in case of error,
1. OutOfMemory : This would exit the application.
2. IOException: There is no action on this exception for now. Only callstack is
printed when -Dprism.verbose=true.
> 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?
I verified this now, by changing native code to always throw an exception from
different error cases. The memory is freed correctly and remains in same range
for any number of images,
PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Thu, 28 Nov 2019 11:12:42 GMT, Arunprasad Rajkumar
wrote:
> On Wed, 27 Nov 2019 11:58:18 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.
>>
>>
>>
>> Commits:
>> - 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.00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8212034
>> Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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
>
> modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1625:
>
>> 1624:
>> 1625: JSAMPROW scanline_ptr = (JSAMPROW) malloc(bytes_per_row * sizeof
>> (JSAMPLE));
>> 1626: if (scanline_ptr == NULL) {
>
> You can remove quite a few calls to `free` if you move the memory allocation
> for `scanline_ptr` just [before it's
> usage](https://github.com/openjdk/jfx/blob/7af932b7f5215949776ec79fb2a5484c521b21a1/modules/javafx.graphics/src/main/native-iio/jpegloader.c#L1690).
> Also free it as soon as you are done with it.
PR is updated according to this comment, please have a look.
PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 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.
>
>
>
> Commits:
> - 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.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8212034
> Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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
modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1625:
> 1624:
> 1625: JSAMPROW scanline_ptr = (JSAMPROW) malloc(bytes_per_row * sizeof
> (JSAMPLE));
> 1626: if (scanline_ptr == NULL) {
You can remove quite a few calls to `free` if you move the memory allocation
for `scanline_ptr` just [before it's
usage](https://github.com/openjdk/jfx/blob/7af932b7f5215949776ec79fb2a5484c521b21a1/modules/javafx.graphics/src/main/native-iio/jpegloader.c#L1690).
Also free it as soon as you are done with it.
PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 21:13:40 GMT, Johan Vos wrote: > On Wed, 27 Nov 2019 11:58:18 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. >> >> >> >> Commits: >> - 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.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 >> Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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 > > modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1345: > >> 1344: free(cinfo->err); >> 1345: free(cinfo); >> 1346: ThrowByName(env, "java/io/IOException", buffer); > > jerr_mgr is also allocated via malloc, but not freed. Do you want to free > that too? Line: 1332=> jerr_mgr ptr is stored in cinfor->err `cinfo->err = jpeg_std_error(&(jerr_mgr->pub));` so free(cinfo->err) is same as free (jerr_mgr). free(cinfo->err) is used here [instead of free(jerr_mgr)] to match the free call in imageio_dispose() method. PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 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. > > > > Commits: > - 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.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 > Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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 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? PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 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. > > > > Commits: > - 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.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 > Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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 modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1345: > 1344: free(cinfo->err); > 1345: free(cinfo); > 1346: ThrowByName(env, "java/io/IOException", buffer); jerr_mgr is also allocated via malloc, but not freed. Do you want to free that too? PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 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. > > > > Commits: > - 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.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 > Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 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 This will need two reviewers. PR: https://git.openjdk.java.net/jfx/pull/54
