Fix for small leak in TimeZone_md.c
Langer, Christoph
christoph.langer at sap.com
Wed Aug 26 21:34:35 UTC 2015
Hi Andrew,
today I have posted the following RFR in i18n-dev:
when working on TimeZone_md.c lately, I found it worthwhile to do some cleanup on it. Please review my changes.
Bug: https://bugs.openjdk.java.net/browse/JDK-8134505
Webrev: http://cr.openjdk.java.net/~goetz/webrevs/8134505-timeZone/webrev.01/
I basically did the following things:
- clean up some typos and comment errors
- added #include "TimeZone_md.h"
- split up the platform specific define sections into an #ifdef <platform1> #elif <platform2> #elif … #endif construct
- moved some AIX coding from the #ifdef block at the bottom of the file into the first AIX specific block
- AIX function “mapPlatformToJavaTimezone”: use a dynamic malloced buffer instead of a fixed length buffer
- refactor function “findJavaTZ_md” to make it more straightforward and to avoid unnecessary mallocs and don’t forget necessary frees
I’m also wondering if the “if (tz == NULL || *tz == '\0') {“ of line 770 could be used for all platforms instead of Solaris and AIX only. The other platforms will only do a check if TZ is NULL but not if it is an empty string.
I did not follow the suggestion to split up "findJavaTZ_md" in platform versions, though.
I'd be grateful for your feedback.
Thanks
Christoph
-----Original Message-----
From: Andrew Hughes [mailto:gnu.andrew at redhat.com]
Sent: Mittwoch, 26. August 2015 20:32
To: Dmitry Samersoff <dmitry.samersoff at oracle.com>
Cc: Langer, Christoph <christoph.langer at sap.com>; Thomas Stüfe <thomas.stuefe at gmail.com>; jdk9-dev at openjdk.java.net
Subject: Re: Fix for small leak in TimeZone_md.c
----- 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
More information about the jdk9-dev
mailing list