<AWT Dev> RFR JDK-8171836: Memory leak in java.desktop/unix/native/common/awt/fontpath.c

[Phil Race]

This looks correct.

+1

I can commit this if we get a 2nd reviewer to approve.

-phil.

On 01/04/2017 11:05 AM, David CARLIER wrote:
> Hi here an updated version. Thanks. regards.
>
>
>>>>>>>> Whilst the loop termination condition of "<  totalDirCount" is fine
>>>>>>>> in the code from which you have copied it, it is not correct here,
>>>>>>>> since you are still in the loop which populates the array.
>>>>>>>> The likely resulting crash will be a lot worse than the leak ..
>>>>>>>>
>>>>>>>> Also I am confused about who is the OCA contributor of this fix, and
>>>>>>>> who is the sponsor (JDK 9 committer) for this fix.
>>>>>>>>
>>>>>>>> And, BTW, this should really have been reviewed on 2d-dev.
>>>>>>>>
>>>>>>>>
>>>>>>>> -phil.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 01/03/2017 12:13 PM, David CARLIER wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> this is a new version.
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>>
>>>>>>>>> On 22 December 2016 at 05:35, Ambarish
>>>>>>>>> Rapte<ambarish.rapte at oracle.com>
>>>>>>>>> wrote:
>>>>>>>>>> Hi Abhijit,
>>>>>>>>>>
>>>>>>>>>>                      There can be references added to newFontPath[]
>>>>>>>>>> in
>>>>>>>>>> previous
>>>>>>>>>> iterations of loop at Line 298: newFontPath[nPaths++] = onePath;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      So these references should also be freed,
>>>>>>>>>> something
>>>>>>>>>> similar
>>>>>>>>>> to code at line 308:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      for ( index = origNumPaths; index<
>>>>>>>>>> totalDirCount;
>>>>>>>>>> index++ )
>>>>>>>>>> {
>>>>>>>>>>
>>>>>>>>>>                                      free( newFontPath[index] );
>>>>>>>>>>
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> As many references added to newFontPath should be freed
>>>>>>>>>> accordingly.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>>
>>>>>>>>>> Ambarish
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> From: Abhijit Roy
>>>>>>>>>> Sent: Thursday, December 22, 2016 12:08 AM
>>>>>>>>>> To: Vadim Pakhnushev; awt-dev at openjdk.java.net
>>>>>>>>>> Subject: Re:<AWT Dev>  RFR JDK-8171836: Memory leak in
>>>>>>>>>> java.desktop/unix/native/common/awt/fontpath.c
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Vadim,
>>>>>>>>>>
>>>>>>>>>> Yes. I did a mistake here. Please find the correct webrev below.
>>>>>>>>>>
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rpatil/8171836/webrev.01/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Abhijit
>>>>>>>>>>
>>>>>>>>>> On 12/21/2016 7:47 PM, Vadim Pakhnushev wrote:
>>>>>>>>>>
>>>>>>>>>> Abhijit,
>>>>>>>>>> I think there's some misunderstanding here.
>>>>>>>>>> The pointer you are trying to free is NULL already:
>>>>>>>>>>
>>>>>>>>>>           if ( newFontPath == NULL ) {
>>>>>>>>>>             free ( ( void *) appendDirList );
>>>>>>>>>> +      free((void*) newFontPath);
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Vadim
>>>>>>>>>>
>>>>>>>>>> On 21.12.2016 16:02, Abhijit Roy wrote:
>>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Please review the fix for the bug below:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8171836
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Description: Memory leak in
>>>>>>>>>> java.desktop/unix/native/common/awt/fontpath.c
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rpatil/8171836/webrev.00/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> To prevent memory leak issue, I have released the newFontPath in
>>>>>>>>>> java.desktop/unix/native/common/awt/fontpath.
>>>>>>>>>>
>>>>>>>>>> Moving forward it for review.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Abhijit
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>