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

[Justin King]

On Thu, 11 May 2023 07:38:45 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> **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. Th...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Second batch of suggestions from @chhagedorn

Marked as reviewed by jcking (Committer).

src/hotspot/share/libadt/dict.hpp line 65:

> 63: 
> 64:   // Allow move constructor for && (eg. capture return of function)
> 65:   Dict(Dict&&) = default;

Nit: You might consider invalidating the other dict being moved from, to catch accidental use-after-move. Could be punted to a future change.

src/hotspot/share/libadt/vectset.hpp line 59:

> 57: 
> 58:   // Allow move constructor for && (eg. capture return of function)
> 59:   VectorSet(VectorSet&&) = default;

Same as the other, consider invalidating the moved from `VectorSet` by setting the data to nullptr or something similar to catch misbehaving code.

src/hotspot/share/opto/node.hpp line 1543:

> 1541: 
> 1542:   // Allow move constructor for && (eg. capture return of function)
> 1543:   Node_Array(Node_Array&&) = default;

Same as other, consider invalidating moved from.

src/hotspot/share/opto/node.hpp line 1572:

> 1570: 
> 1571:   // Allow move constructor for && (eg. capture return of function)
> 1572:   Node_List(Node_List&&) = default;

Same as other, consider invalidating moved from.

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

PR Review: https://git.openjdk.org/jdk/pull/13833#pullrequestreview-1428947355
PR Review Comment: https://git.openjdk.org/jdk/pull/13833#discussion_r1195400920
PR Review Comment: https://git.openjdk.org/jdk/pull/13833#discussion_r1195404283
PR Review Comment: https://git.openjdk.org/jdk/pull/13833#discussion_r1195405926
PR Review Comment: https://git.openjdk.org/jdk/pull/13833#discussion_r1195405837