RFR: 8359200: Memory corruption in MStack::push

[Marc Chevalier]

On Wed, 11 Jun 2025 11:49:08 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> I found this by accident when running testing with non-default `-XX:OptoNodeListSize` (see JBS for details). The problem is that `MStack::push` assumes that `Node_Stack::grow` will always grow the stack by at least two (line 69) and it then proceeds to put two items in:
> https://github.com/openjdk/jdk/blob/db6fa5923cd0394dfb44c7e46c3e7ccc102a933a/src/hotspot/share/opto/matcher.hpp#L67-L74
> 
> But after [JDK-8336999](https://bugs.openjdk.org/browse/JDK-8336999), `Node_Stack::grow` will only grow the stack if needed:
> https://github.com/openjdk/jdk/blob/db6fa5923cd0394dfb44c7e46c3e7ccc102a933a/src/hotspot/share/opto/node.cpp#L3027-L3031
> 
> However, if there's **one** empty slot, the stack will not be grown and `MStack::push` then puts **two** items on the stack which leads to memory corruption.
> 
> I refactored the push method to delegate to `Node_Stack::push` which does the right thing and, similar to [JDK-8343056](https://bugs.openjdk.org/browse/JDK-8343056), also added `maybe_grow` methods for all the containers affected by the original change. For additional coverage, I moved the `_nesting.check` calls to before the check that determines if we grow.
> 
> I only ever observed this with a non-default and odd value for `-XX:OptoNodeListSize` but I'm not 100% convinced that it can't happen with the default value, so I'm treating this as P2 and will backport the fix to JDK 25.
> 
> @shipilev Since you worked on [JDK-8343056](https://bugs.openjdk.org/browse/JDK-8343056), could you please take a look at this?
> 
> Thanks,
> Tobias

I like the double push, it looks simpler, safer. Hopefully, the C++ compiler is smart enough to make that just as efficient.

Overall, a lot of checks (like `_nesting.check(_set_arena);`) are moved from `grow` to `maybe_grow`, as stated in the description, but also things that looks more necessary like `if (i >= Max())` in `Block_Array::maybe_grow`. So, I suppose one shouldn't use `grow` directly, and you indeed changed `Block_Array::map` this way. But what about derived classes? `grow` is only protected in most (all?) of these classes, so some derived classes could call it. Is it worth making it private? Or maybe calling `grow` directly is not so wrong?

Overall, it's not clear to me whether calling `grow` directly is wrong (and then not very discouraged, maybe making it private or commenting about not using it would help), or whether I just lack imagination and `grow` makes sense to be called not only through `maybe_grow`. And indeed, if `grow` must be called only through `maybe_grow` one could inline it, and yet, it's not (for classes that had both). Or maybe it's a well known thing not to do that, and I'm just inventing mistakes nobody would make in real life. Just asking!

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

PR Comment: https://git.openjdk.org/jdk/pull/25751#issuecomment-2962602947