Quantcast

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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

DEV Nexen
Hi,

there is the original patch indeed it is not this.

Kindest regards.

On 21 December 2016 at 14:17, Vadim Pakhnushev
<[hidden email]> 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
>
>
>
>

patch-memerror.diff (754 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Abhijit Roy
In reply to this post by Vadim Pakhnushev

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

 



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ambarish Rapte

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; [hidden email]
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

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

DEV Nexen
Hi all,

this is a new version.

Cheers,

On 22 December 2016 at 05:35, Ambarish Rapte <[hidden email]> 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; [hidden email]
> 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
>
>
>
>
>
>

patch-memerror.diff (948 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Philip 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 <[hidden email]> 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; [hidden email]
>> 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
>>
>>
>>
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

DEV Nexen
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<[hidden email]>
>>>>>>>> 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; [hidden email]
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>

patch-memerror.diff (930 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Philip 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<[hidden email]>
>>>>>>>>> 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; [hidden email]
>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ambarish Rapte
Hi David,
+1
Looks good to me.

Regards,
Ambarish

-----Original Message-----
From: Phil Race
Sent: Thursday, January 05, 2017 12:39 AM
To: David CARLIER; [hidden email]
Subject: Re: <AWT Dev> RFR JDK-8171836: Memory leak in java.desktop/unix/native/common/awt/fontpath.c

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<[hidden email]>
>>>>>>>>> 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; [hidden email]
>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

DEV Nexen
Thanks for the time spent ;-)

On 5 January 2017 at 08:08, Ambarish Rapte <[hidden email]> wrote:
Hi David,
+1
Looks good to me.

Regards,
Ambarish

-----Original Message-----
From: Phil Race
Sent: Thursday, January 05, 2017 12:39 AM
To: David CARLIER; [hidden email]
Subject: Re: <AWT Dev> RFR JDK-8171836: Memory leak in java.desktop/unix/native/common/awt/fontpath.c

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<[hidden email]>
>>>>>>>>> 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; [hidden email]
>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>


Loading...