RFR: 8212034: Potential memory leaks in jpegLoader.c in error case

[Ambarish Rapte]

On Mon, 2 Dec 2019 12:14:21 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

> On Thu, 28 Nov 2019 11:12:42 GMT, Arunprasad Rajkumar <arajkumar at openjdk.org> wrote:
> 
>> On Wed, 27 Nov 2019 11:58:18 GMT, Ambarish Rapte <arapte at openjdk.org> 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