Skip to content
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

8212034: Potential memory leaks in jpegLoader.c in error case #54

Open
wants to merge 2 commits into
base: master
from

Conversation

@arapte
Copy link

arapte commented Nov 27, 2019

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.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JDK-8212034: Potential memory leaks in jpegLoader.c in error case

@bridgekeeper

This comment has been minimized.

Copy link

bridgekeeper bot commented Nov 27, 2019

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Nov 27, 2019
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Nov 27, 2019

Webrevs

@kevinrushforth kevinrushforth self-requested a review Nov 27, 2019
@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Nov 27, 2019

This will need two reviewers.

@@ -1338,6 +1341,8 @@ JNIEXPORT jlong JNICALL Java_com_sun_javafx_iio_jpeg_JPEGImageLoader_initDecompr
char buffer[JMSG_LENGTH_MAX];
(*cinfo->err->format_message) ((struct jpeg_common_struct *) cinfo,
buffer);
free(cinfo->err);
free(cinfo);

This comment has been minimized.

Copy link
@johanvos

johanvos Nov 27, 2019

Collaborator

jerr_mgr is also allocated via malloc, but not freed. Do you want to free that too?

This comment has been minimized.

Copy link
@arapte

arapte Nov 28, 2019

Author

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.

Copy link
Collaborator

johanvos left a comment

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?

int offset = 0;

JSAMPROW scanline_ptr = (JSAMPROW) malloc(bytes_per_row * sizeof (JSAMPLE));

This comment has been minimized.

Copy link
@arajkumar

arajkumar Nov 28, 2019

Contributor

You can remove quite a few calls to free if you move the memory allocation for scanline_ptr just before it's usage. Also free it as soon as you are done with it.

This comment has been minimized.

Copy link
@arapte

arapte Dec 2, 2019

Author

PR is updated according to this comment, please have a look.

This comment has been minimized.

Copy link
@arapte

arapte Dec 2, 2019

Author

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,

Ambarish Rapte
@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 24, 2020

/reviewers 2

@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Jan 24, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.