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

Tue Sep 8 16:39:35 UTC 2020

Hi Andrew,

updated webrev https://cr.openjdk.java.net/~apetushkov/8252904.01/

Andrey

On 08.09.2020 19:22, Andrey Petushkov wrote:
> 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