[8u] RFR 8252904: VM crashes when JFR is used and JFR event class is transformed

[Andrew Dinn]

Hi Andrey,

Thanks for debugging this and great work pinning it down.

I think this problem is probably responsible for the following Graal issue

  https://github.com/oracle/graal/issues/2748

On 08/09/2020 12:23, Andrey Petushkov wrote:
> Please consider the following fix to avoid crash when JFR event class is
> transformed upon load. The problem is caused by using stale reference to
> ClassFileStream which is created (conditionally if the class is
> transformed by JVMTI hook) under ResourceMark in
> ClassFileParser.parseClassFile(). This dead reference is leaked into
> caller code, where it is accessed by JFR itself.
> 
> There is no such problem in jdk11 and above because class loading
> implementation is completely different and JVMTI hook responsible for
> transformation is invoked by KlassFactory, not ClassFileParser.
> Moreover, in jdk11 it's KlassFactory (analog of
> ClassLoader/SystemDictionary in a sense that it's the caller of
> ClassFileParser is responsible for placing ResourceMark, just like
> proposed patch does for jdk8u code)
> . . .> Webrev is here https://cr.openjdk.java.net/~apetushkov/8252904/
I don't much like this solution because it places the ResourceMark
outside of the scope in which the allocations it guards against are
performed i.e. at around the call to parseClassFile rather than at the
start of that method.

I know there is a precedent for this in the way the jdk11u code works.
However, it's slightly less opaque in that case as the code that
allocates (and, potentially re-allocates) the stream is only ever
invoked under a constructor which is located in the same scope as the
ResourceMark.

That said, I don't see any easy way to achieve the desired outcome
without a significant re-ordering of the jdk8u code which risks
introducing other problems. We certainly don;t want to backport the
significant changes made in jdk11u. So, I guess we are stuck with doing
it this way.

I have 3 other concerns. You allocated a ResourceMark before the calls
to parseClassFile in  SystemDictionary::parse_stream and
SystemDictionary::resolve_from_stream but not before the call from
ClassLoader::load_classfile. Surely that also needs one?

Secondly, the ResourceMark in SystemDictionary::parse_stream and
SystemDictionary::resolve_from_stream both extend to the end of the
method. Clearly, in the second case the scope needs to extend beyond the
call to ON_KLASS_CREATION. However, is it necessary to extend to the end
of the caller method? I think it would be be better to declare each
ResourceMark in a new block that limits the scope to the minimal range
that is needed (if you can do that without updating the current
indentation that would be even better).

Finally, I think you need to add more commentary to explain what is
going on. So, you need a comment at each point where a ResourceMark is
allocated saying:

// Callers are expected to declare a ResourceMark to determine
// the lifetime of any updated (resource) allocated under
// this call to parseClassFile

In the method itself you should say more in the comment which replaces
the ResourceMark declaration:

// JDK-8252904:
// The stream (resource) attached to the instance klass may
// be reallocated by this method. When JFR is included the
// stream may need to survive beyond the end of the call. So,
// the caller is expected to declare the ResourceMark that
// determines the lifetime of resources allocated under this
// call.

Last but not least, what have you done to test this patch?

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill