Fix for small leak in TimeZone_md.c

[Dmitry Samersoff]

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.