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

[Phil Race]

+            for ( index = origNumPaths; index < totalDirCount; index++ ) {
+                free( newFontPath[index] );
+            }


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
>>
>>
>>
>>
>>
>>