RFR: 8132725: Memory leak in Arguments::add_property function

[Ioi Lam]

Hi Dmitry,

Is this change part of 8132725?

3904   jint code = set_aggressive_opts_flags();
3905   if (code != JNI_OK) {
3906     return code;
3907   }


1041       if (_java_vendor_url_bug != DEFAULT_VENDOR_URL_BUG) {

 >> also check (_java_vendor_url_bug != NULL) for sanity?


Also, there's a lot of duplicated "if (eq != NULL) { FreeHeap((void 
*)key);}". Maybe these can be consolidated with a "goto"? I know lots of 
people haye goto but it will make the clean up less error prone:

bool Arguments::add_property(const char* prop) {
     ....
     bool status = false;
     ....
        char *_java_command_new = os::strdup(value, mtInternal);
        if (_java_command_new == NULL) {
          goto done;
        }else {
          if (_java_command != NULL) {
            os::free(_java_command);
          }
          _java_command = _java_command_new;
        }
     ....
     }
     // Create new property and add at the end of the list
     PropertyList_unique_add(&_system_properties, key, value);
   }
   status = true;

done:
    if (key != prop) {
      // SystemProperty copy passed value, thus free previously allocated
      // memory
     FreeHeap((void *)key);
    }
    return status;
}

Also, using (key != prop) would make the code clearer than (eq != NULL).

Thanks
- Ioi

On 8/13/15 12:55 AM, Dmitry Dmitriev wrote:
> Hello,
>
> Please review this fix which remove memory leak in 
> Arguments::add_property function. Also, I need a sponsor for this fix, 
> who can push it.
>
> Arguments::add_property function allocate memory for key and value. 
> Then key and values are passed to the PropertyList_unique_add function 
> which use SystemProperty class to add or update property value. 
> SystemProperty class maintains it's own copy of key and value and thus 
> copy passed key and value. Therefore key and value must be freed in 
> add_property function(with exception for value in case of 
> "java.vendor.url.bug" and "sun.java.command" properties).
>
> In this fix I allocate memory only for key when passed property 
> contains value. If passed property not contains value, then I not 
> allocate memory for key and use passed property string. Value also 
> extracted from passed property string instead of allocating. To 
> accomplish that I changed declaration of "value" in several functions 
> from "char *" to  "const char *" since value is not modified in these 
> functions(PropertyList_* functions, SystemProperty class methods).
>
> Processing of "java.vendor.url.bug" and "sun.java.command" properties 
> also corrected. Now when these properties redefined, then code checks 
> if memory was allocated for special variables of these 
> properties(checking that not contains default value) and free it.
>
> Webrev: http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.00/ 
> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.00/>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8132725
> Tested: JPRT(hotspot test set), hotspot all, vm.quick
>
> Thanks,
> Dmitry