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

[Thomas Stüfe]

Hi Andrew,

okay, looks good.

..Thomas

On Fri, Apr 3, 2020 at 5:58 AM Andrew Hughes <gnu.andrew at redhat.com> wrote:

> 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
>
>