Fix for small leak in TimeZone_md.c

[Thomas Stüfe]

Hi Christoph,

On Thu, Aug 20, 2015 at 10:22 AM, Langer, Christoph <
christoph.langer at sap.com> wrote:

> Hi Thomas, Andrew and Dmitry,
>
> thanks for your comments - they are very valuable and lead me to the
> following:
>
> 1. The initial change which just fixes the sport where the Solaris leak
> could occur should be commited as fix to bugreport
> https://bugs.openjdk.java.net/browse/JDK-8133830
> I'll do a separate posting for review
>

That makes sense, thanks. I can open a separate issue for the AIX cleanup,
if you want.

Thomas


> 2. I will attempt to do a better refactoring of the "findJavaTZ_md" code
> for better readability and to avoid unnecessary string copy operations.
> Will post a new webrev later.
>
> Thanks & Best regards
> Christoph
>
>
> -----Original Message-----
> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
> Sent: Mittwoch, 19. August 2015 23:17
> To: Andrew Hughes
> Cc: Langer, Christoph; Thomas Stüfe; jdk9-dev at openjdk.java.net
> Subject: Re: Fix for small leak in TimeZone_md.c
>
> Andrew,
> > So it would need further refactoring rather than simply being dropped.
> Agree.
> > I do wonder why it needs to be copied at all when tz itself is the
> > result of a strdup from the call to getPlatformTimeZoneID,
> > getSolarisDefaultZoneID or mapPlatformToJavaTimezone.
> If getenv("TZ") returns some value the code just return this value
> (except AIX and "localtime" under Solaris) and it has to be strdup'ed
> before return.
>
> It might be better to create separate function for each platform despite
> some code duplication. It makes the code better readable.
>
> -Dmitry
>
>
> On 2015-08-19 23:51, Andrew Hughes wrote:
> >
> > ----- Original Message -----
> >> Lander,
> >>
> >> My few cents ...
> >>
> >> 1. After you refactoring freetz is never actually used. It might be
> >> better to just get rid of this variable.
> >>
> > At present, it's still being used to hold the old version of tz:
> >
> >         tz = getPlatformTimeZoneID();
> >         freetz = tz;
> >
> > So both tz and freetz point to the return value of getPlatformTimeZoneID.
> > tz may then be moved forward due to:
> >
> >     if (tz != NULL && *tz == ':') {
> >         tz++;
> >     }
> >
> > and then it is replaced altogether:
> >
> >     if (tz != NULL && strcmp(tz, "localtime") == 0) {
> >         tz = getSolarisDefaultZoneID();
> >
> > Now tz contains the return value of getSolarisDefaultZoneID, so only
> > freetz references the old return value from getPlatformTimeZoneID.
> > This code then frees it:
> >
> >         if (freetz != NULL) {
> >             free((void *) freetz);
> >         }
> >         freetz = tz;
> >
> > and tz and freetz match again.
> >
> > You could get rid of freetz if tz was freed before being overwritten
> > in this case, but the same isn't true in the AIX case where tz is
> > passed to mapPlatformToJavaTimezone.
> >
> > So it would need further refactoring rather than simply being dropped.
> >
> >> 2. Also you can remove javatz as well and further refactor the code to
> >>
> >>     return (tz == NULL) ? NULL : strdup(tz);
> >>
> > At present, tz, as referenced by freetz, is freed after the strdup.
> > I do wonder why it needs to be copied at all when tz itself is the
> > result of a strdup from the call to getPlatformTimeZoneID,
> > getSolarisDefaultZoneID or mapPlatformToJavaTimezone.
> >
> >> -Dmitry
> >>
> >> On 2015-08-19 16:14, Langer, Christoph wrote:
> >>> Hi Thomas,
> >>>
> >>> thanks for your review.
> >>>
> >>> Axel already helped me to create a bug report:
> >>> https://bugs.openjdk.java.net/browse/JDK-8133830
> >>>
> >>> I had also updated my webrev, as I would like to take the chance to do
> some
> >>> minor refactoring of the AIX porting code for better readability:
> >>> http://cr.openjdk.java.net/~asiebenborn/8133830/webrev/
> >>>
> >>> Can you review the updated change as well?
> >>>
> >>> Thanks
> >>> Christoph
> >>>
> >>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> >>> Sent: Mittwoch, 19. August 2015 13:12
> >>> To: Langer, Christoph
> >>> Cc: jdk9-dev at openjdk.java.net
> >>> Subject: Re: Fix for small leak in TimeZone_md.c
> >>>
> >>> Hi Christoph,
> >>>
> >>> looks fine. I opened a bug for you:
> >>> https://bugs.openjdk.java.net/browse/JDK-8133933
> >>>
> >>> But this needs another reviewer and a sponsor.
> >>>
> >>> Kind Regards, Thomas
> >>>
> >>>
> >>>
> >>> On Tue, Aug 18, 2015 at 11:52 AM, Langer, Christoph
> >>> <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:
> >>> Hi all,
> >>>
> >>> I think there is a small memory leak in TimeZone_md.c in the case of
> >>> solaris "localtime".
> >>>
> >>> When getPlatformTimeZoneID() is called and its result is "localtime",
> the
> >>> returned string buffer is not freed as the pointer "freetz" is
> overwritten
> >>> after the call to getSolarisDefaultZoneID().
> >>>
> >>> Please have a look at my webrev:
> >>> http://cr.openjdk.java.net/~asiebenborn/christoph/webrev/ for a
> potential
> >>> fix.
> >>>
> >>> A bug was not yet created but should be done after you agree that this
> is
> >>> an issue.
> >>>
> >>> Thanks in advance for comments.
> >>>
> >>> Best regards
> >>> Christoph
> >>>
> >>
> >> --
> >> Dmitry Samersoff
> >> Oracle Java development team, Saint Petersburg, Russia
> >> * I would love to change the world, but they won't give me the sources.
> >>
> >>
>
>
> --
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.
>
>