RFR: 8302670: use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack [v11]

Emanuel Peter epeter at openjdk.org
Wed May 24 11:09:35 UTC 2023


> **Motivation**
> 
> - Generally: we copy containers by value: the consequence is that we copy all internals by value, including size, capacity and pointers. From there on, the containers can diverge, and make each other inconsistent. One may be destructed and free the memory of the other one. In theory this could cause a bug on the main-branch. In practice, we probably (maybe?) use the correct one of the many copies that is currently supposed to be alive. If one pushes to the wrong copy, then one will most likely eventually hit a SIGSEGV - which has happened to me and @TobiHartmann a few times - it is very annoying. Plus: copy by value of containers is very bad design, and makes it difficult to understand which one is the "live" copy.
> - We also overwrite igvn phases. One case is particularly hairy: `igvn = ccp` (truncate ccp, and store it into igvn variable. Aka `object slicing in c++`)
> 
> @jcking 's first version https://github.com/openjdk/jdk/pull/12703. He dropped it as a "discussion or starting point for somebody else". I took a lot of ideas from him, but went a bit more aggressive with refactoring instead of the `replace_with` move-like approach.
> 
> **Changes**
> 
> - Make many containers `NONCOPYABLE`:
>   - `Dict`
>   - `VectorSet`
>   - `Node_Array`, `Node_List`, `Unique_Node_List`
>   - `Node_Stack`
>   - `NodeHash`
>   - `Type_Array`
>   - `Phase`
>   - Note: for many classes I still allow the `A(A&&) = default;` constructor. This allows implicit moves (rvalues) so that we can return containers from functions and capture them.
> - Create "global" containers for `Compile`:
>   - `C->igvn_worklist()` (renamed from `for_igvn`, referenced to by `PhaseIterGVN._worklist`)
>   - `C->type_array()` (referenced to by `PhaseValues._types`)
>   - `C->node_hash_table()` (referenced to by `PhaseValues._table`)
>   - They are created in the `Compile` constructor. The phases can then hold a reference (`&`) to them.
>   - Note: before, these were located in the phases, and passed back and forth by value. They were passed downward via the phase constructor, where the corresponding fields were taken over from the previous phase. Then they were passed upward by `PhaseGVN.replace_with` (for `_table` and `_types`), or by simply overwriting the old `igvn` variable with a newly constructed igvn that has the containers passed into its constructor from the previous phase. I imagine it as "weaving" the containers from phase to phase, where the ownership travels. The messy part was that oth...

Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits:

 - Merge branch 'master' into JDK-8302670
 - explicitly deleting the move assign operator
 - Second batch of suggestions from @chhagedorn
 - Apply suggestions from @chhagedorn
   
   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
 - Last of 4 suggestion commits from @TobiHartmann
 - make igvn_worklist() from ref to pointer
 - renamed _type_array to types, and _node_hash_table to _node_hash
 - Suggestions by @TobiHartmann
   
   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
 - adding comment back in
 - add removed deconstructors back in, just to be sure we do not change behavior
 - ... and 16 more: https://git.openjdk.org/jdk/compare/2d4d8508...3404ec14

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

Changes: https://git.openjdk.org/jdk/pull/13833/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13833&range=10
  Stats: 717 lines in 37 files changed: 204 ins; 296 del; 217 mod
  Patch: https://git.openjdk.org/jdk/pull/13833.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13833/head:pull/13833

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


More information about the hotspot-dev mailing list