<i18n dev> Fix for small leak in TimeZone_md.c

[Langer, Christoph]

Hi Roger,

thanks for your comments.

I agree with you and I will remove the platform specifics around line 770.

As for the suggestions about line 804 and beyond, it would perhaps be nicer to have mutually exclusive sections. However, then I would have to add the common, "not _AIX and not __solaris__", code also to the "__solaris__" part for the case that tz does not equal "localtime", because then the common handling would apply as well. So, to avoid code duplication I'd prefer to leave it as it is.

Best regards
Christoph
 

-----Original Message-----
From: jdk9-dev [mailto:jdk9-dev-bounces at openjdk.java.net] On Behalf Of Roger Riggs
Sent: Dienstag, 1. September 2015 23:46
To: jdk9-dev at openjdk.java.net
Subject: Re: Fix for small leak in TimeZone_md.c

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