Fix for small leak in TimeZone_md.c

[Andrew Hughes]

----- Original Message -----
> 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.

Yes, I get that, but it appears to happen twice; once in the function
that obtains the value and then once again in the calling function.

> 
> It might be better to create separate function for each platform despite
> some code duplication. It makes the code better readable.

+1. I don't think there's enough duplication here to warrant how unreadable
the code is.

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

-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

PGP Key: rsa4096/248BDC07 (hkp://keys.gnupg.net)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07