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

[Dean Long]

On Mon, 5 Feb 2024 16:02:57 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.

Back when I filed JDK-8132354, I proposed that both C1 and C2 delegate to ciEnv.  Now there is a 3rd layer/scope of error reporting.  I don't think the solution needs to be so complicated.  Even with the new formatted strings, the maximum possible length can be determined (I don't see it using klass or method names).  So why can't the storage simply be `char[MAX_COMPILER_ERROR_LEN]` in `CompilerThread`, and setting it be a `strncpy`?

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

PR Comment: https://git.openjdk.org/jdk/pull/17710#issuecomment-1945544012