RFR: 8325095: C2: bailout message broken: ResourceArea allocated string used after free

[Vladimir Kozlov]

On Thu, 15 Feb 2024 18:04:20 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> **Problem**
>> Bailout `failure_reason` strings used to be either `nullptr` or a static string. But with [JDK-8303951](https://bugs.openjdk.org/browse/JDK-8303951) I had introduced dynamic strings (e.g. formatted using `stringStream::as_string()`). These dynamic strings were ResourceArea allocated, and have a very limited life-span, i.e. withing the most recent ResourceMark scope. The pointer is passed to `Compile::_failure_reason`, which already might leave that ResourceMark scope, and the pointer is invalid. And then the pointer is further passed to `ciEnv::_failure_reason`. And finally, it even gets passed up to `CompileBroker::invoke_compiler_on_method`. When the string is finally printed for the bailout message, we already left original `ResourceMark` scope, and also the `Compile` and `ciEnv` scopes. The pointer now points to basically random data.
>> 
>> **Solution**
>> Whenever a string is passed to an outer scope, I make a CHeap copy, and the outer scope is the owner of that copy.
>> This way every scope is the owner of its own copy, and can allocate and deallocate according to its own strategy safely.
>> 
>> I introduced a utility class `CHeapStringHolder`:
>>  - `set`: make local copy on CHeap.
>>  - `clear`: free local copy. We before `set`, and in the destructor. Thus, when the holder goes out of scope, the memory is automatically freed.
>> 
>> We have these 4 scopes:
>> - `ResourceMark`: It allocates from `ResourcArea`, and deallocation is taken care at the end of the ResourceMark scope.
>> - `Compile`: We turn the `_failure_reason` from a `char*` into a `CHeapStringHolder`. We set `_failure_reason` while the `ResourceMark` is live.
>> - `ciEnv`: We turn the `_failure_reason` from a `char*` into a `CHeapStringHolder`. We set `_failure_reason` while `Compile` is live.
>> - `CompileTask`: We used to just set `failure_reason`, which assumes that the string is static or elsewhere managed. Now I use pattern available in other places of `CompileBroker::invoke_compiler_on_method`: duplicate the string with `os::strdup` from some other scope, and set `reason_on_C_heap = true`, which means that we are the owner of that copy, and are responsible for freeing it later. Not sure if that is a nice pattern, but I don't want to refactor that code and it does what I need it to do.
>
> src/hotspot/share/ci/ciEnv.cpp line 1272:
> 
>> 1270:     // Reset failure reason; this one is more important.
>> 1271:     _failure_reason.clear();
>> 1272:     record_failure(reason);
> 
> @dean-long the tricky part is here, I think:
> We are in `ciEnv::record_method_not_compilable`. Depending on above condition, we either update the failure reason, or don't. So there may already be an old failure reason (the first one) that we overwrite (with a second one) or leave.
> 
> This method is called from `Compile::record_method_not_compilable`. There, we also call `Compile::record_failure`, which sets `Compile::_failure_reason`, with no such condition.
> 
> We might for example decide to keep the old failure reason at ciEnv, but update the failure reason at Compile, at least that is how I read the code.
> 
> Hence, there can be multiple failure reasons around, I think. And I cannot fit multiple messages in a single buffer. Thus, I need some way to dynamically allocate.
> 
> Now: is this good or desirable? I don't know. Potentially having different failure messages at Compile and ciEnv - not sure if that is reasonable. We should probably discuss that. But for now I'm just trying to fix the code the way it works.
> 
> What do you think?

I think initially we care only about "most" important failure because implementation allowed to report only one failure.
If you can keep all failures AND report all of them it would be nice, I think. May be in separate RFE.

We should have only one place where we keep failures.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17710#discussion_r1491714124