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

Emanuel Peter epeter at openjdk.org
Thu Feb 15 06:54:21 UTC 2024


**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.

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

Commit messages:
 - strcmp for MemStat
 - cleanup debug prints
 - Merge branch 'master' into JDK-8325095
 - more fix
 - copy string from ciEnv
 - remove use of resource_area
 - fixed ciEnv
 - add string holder class
 - use ResourceArea instead of Arena, to make lifetime smaller
 - 8325095

Changes: https://git.openjdk.org/jdk/pull/17710/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17710&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8325095
  Stats: 140 lines in 9 files changed: 118 ins; 3 del; 19 mod
  Patch: https://git.openjdk.org/jdk/pull/17710.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17710/head:pull/17710

PR: https://git.openjdk.org/jdk/pull/17710


More information about the hotspot-dev mailing list