RFR: [8u] JDK-6424123: "JVM crashes on failed 'strdup' call"

[Andrew Hughes]

On 02/04/2020 08:25, Thomas Stüfe wrote:
> Hi Andrew,
> 
> some small issues (not a 8u reviewer though):
> 
> ----
> 
> os_aix.cpp: 
>    size_t data_page_size = SIZE_4K;
>    {
> -    void* p = ::malloc(SIZE_16M);
> +    void* p = os::malloc(SIZE_16M, mtInternal);
>      guarantee(p != NULL, "malloc failed");
>      data_page_size = os::Aix::query_pagesize(p);
> -    ::free(p);
> +    os::free(p);
>    }
> 
> This seems wrong. We should explicitly use raw malloc here.
> 
> We do so in head. I see that we rolled this part back with 8075505 which
> was a larger change revamping AIX memory handling. I would leave this
> part out.
> 

Ok, dropped.

> ----
> 
>  #ifdef SPARC
> -          _masm->_verify_oop(r->as_Register(), strdup(st.as_string()),
> __FILE__, __LINE__);
> +          _masm->_verify_oop(r->as_Register(),
> os::strdup(st.as_string(), mtCompiler), __FILE__, __LINE__);
>  #else
> 
> This seems to be a memory leak, but it has been one before that change.
> 
> ---
> 
> classLoader.cpp
> 
> @@ -536,11 +540,11 @@
>          }
>  
>          default:
>          {
>            if (!skipCurrentJar && cur_entry != NULL) {
> -            char* new_name = strdup(package_name);
> +            char* new_name = os::strdup_check_oom(package_name);
>              boot_class_path_packages.append(new_name);
>            }
>          }
>        }
>      }
> 
> I believe this leaks too, and did so before.
> 

Right. This is a backport. I didn't write any of these changes, or alter
them, so if they're wrong, they were wrong in the original patch too.

In the first case, the line is in verification code for non-product
builds, so I suspect we don't care greatly. In the second,
boot_class_path_packages is a GrowableArray, so it looks like new_name
will be freed when clear_and_dellocate is called by its destructor.

> ---
> 
> share/vm/compiler/compilerOracle.cpp
> 
> @@ -217,11 +218,11 @@
>                             Symbol* method_name, Mode method_mode,
>                             Symbol* signature, const char* opt,
>                             const T value,  MethodMatcher* next) :
>      MethodMatcher(class_name, class_mode, method_name, method_mode,
> signature, next),
>                    _type(get_type_for<T>()), _value(copy_value<T>(value)) {
> -    _option = strdup(opt);
> +    _option = os::strdup_check_oom(opt);
>    }
>  
>    ~TypedMethodOptionMatcher() {
>      free((void*)_option);
>    }
> 
> Unmatched free. free must be os::free.

This was missed, because the line was already there rather than being
added by this patch.  Fixed.

New webrev: https://cr.openjdk.java.net/~andrew/openjdk8/6424123/webrev.02

> 
> ---
> 
> Cheers, Thomas
> 

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222