Fix for small leak in TimeZone_md.c
Roger Riggs
Roger.Riggs at Oracle.com
Tue Sep 1 21:45:42 UTC 2015
Hi,
Some comments:
- Line 770: the platform specific conditional compilation can be
removed, if *tz is zero, its not a real tz anyway.
- Line 804: " } else" having the "else" inside the conditional
compilation makes the code more fragile.
Perhaps just return javatz and not have to figure out whether those
other conditions apply.
- It might be clearer if the AIX, Solaris, and linux tail of the code
was mutually exclusive.
i.e.
#if defined(_AIX)
/* On AIX do the platform to Java mapping. */
javatz = mapPlatformToJavaTimezone(java_home_dir, tz);
if (freetz != NULL) {
free((void *) freetz);
}
#elif defined(__solaris__)
/* Solaris might use localtime, so handle it here. */
if (strcmp(tz, "localtime") == 0) {
javatz = getSolarisDefaultZoneID();
if (freetz != NULL) {
free((void *) freetz);
}
}
#else /* otherwise, not _AIX and not __solaris__ */
if (freetz == NULL) {
/* strdup if we are still working on getenv result. */
javatz = strdup(tz);
} else if (freetz != tz) {
/* strdup and free the old buffer, if we moved the pointer. */
javatz = strdup(tz);
free((void *) freetz);
} else {
/* we are good if we already work on a freshly allocated
buffer. */
javatz = tz;
}
#endif
$.02, Roger
On 8/26/2015 5:34 PM, Langer, Christoph wrote:
> 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