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

[Andrey Petushkov]

Hi Andrew,

Thank you very much for your comprehensive review, please see comments
inline

On 08.09.2020 18:34, Andrew Dinn wrote:
> 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.
fully agree with you. And given that it's very much unlikely that the
files in question be modified significantly in the future so I think the
risk of reducing maintainability/adding more opportunities for the bugs
by implementing this change is rather very low
>
> 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?
ClassLoader::load_classfile has it's own ResourceMark in the first line
of it's body. Clear it's done for other reasons but I thought it's close
enough to parseClassFile() invocation statement so I think it's enough
cover maintainability aspects (although adding comment is necessary).
>From the memory allocation aspects I also see no reasons to create
separate allocation context specifically for ClassFileParser. That's why
I did not add another ResourceMark there
>
> 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).
On a second thought I think the proper way is a bit  different. Given
that ClassFileParser instance may hold the reference to memory allocated
in parseClassFile I think that:
- ResourceMark should be put before invocation of the constructor of
ClassFileParser (i.e. declaration of a local variable)
- and extend to the whole scope of ClassFileParser instance

Given that:
- in SystemDictionary::parse_stream I will do exactly as you suggest
because really ClassFileParser lifespan is a single statement
- in SystemDictionary::resolve_from_stream I have to put ResourceMark
one line higher, before introducing `parser` local. However adding an
explicit scope end for ResourceMark as you suggest does not seem to make
any difference here. If done, it shall happen on line 1171 but there is
only debug_only block. So it will anyway span to the end of method (in a
product build). However for the purpose of giving a proper message to
the developers I have no objections in introducing a block here as well.
Please let me know what you think
>
> 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.
Will do
>
> Last but not least, what have you done to test this patch?
We have a proprietary though simple test which instruments all the java
classes loaded into VM. I've used this test in conjunction with the
slowdebug VM to test the fix. As mentioned, the VM crashes reliably
without a fix, while successfully working with it (given the nature of
the test it in fact verifies not only that JFR event classes can now be
instrumented successfully, but also serves some sanity check that all
other class loads happened via other code paths are not affected, to the
extent the slowdebug VM assertions can verify)

I cannot publish the source code of a test but if necessary I can
prepare a minimal reproducer

Thanks,
Andrey
>
> 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