RFR: JDK-8263557: Possible NULL dereference in Arena::destruct_contents()

[Thomas Stuefe]

On Sun, 14 Mar 2021 19:52:51 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Trivial.
>> 
>> Sonarcloud reports a possible access to a NULL C++ object in Arena::destruct_contents():
>> 
>> _first->chop();
>> 
>> I have found no code path where this could happen but _first could conceivably be NULL after a call to Arena::reset(). Lets fix that.
>> 
>> GA test error on windows seems unrelated.
>
> Arena::move_contents leaves _first == nullptr. ~Arena calls destruct_contents. Calling ~Arena on a moved-from arena would hit this. So change looks good, though I'm not so sure about "trivial".
> 
> I'm not sure why we don't hit this.  C2 (in Matcher::match) calls move_contents, but I couldn't figure out what it did with the old (moved-from) arena after that.

Thanks Kim, this is not trivial, I'll remove the marker.

> Arena::move_contents leaves _first == nullptr. ~Arena calls destruct_contents. Calling ~Arena on a moved-from arena would hit this. So change looks good, though I'm not so sure about "trivial".
> 
> I'm not sure why we don't hit this. C2 (in Matcher::match) calls move_contents, but I couldn't figure out what it did with the old (moved-from) arena after that.

Yes, the only code moving arena content around is Matcher::match():

in matcher.cpp:330, we evacuate Compile::node_arena to Compile::old_node_arena, now Compile::node_arena::_first = NULL;

But further down in that function we proceed doing things which cause allocation of new nodes, eg. matcher.cpp:353 Matcher::xform()->Node::clone() which allocates the new node from the node arena. So I think that the node arena gets re-filled immediately.

I am not sure if that happens *always*. No, it probably does not, at least not in the case of a native OOM. But destroying an Arena which just had been evacuated would at least be very rare.

The other uses of Arena (HandleArena and ResourceArea) are straightforward and content never gets moved around, so they never run into this.

Cheers, Thomas

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

PR: https://git.openjdk.java.net/jdk/pull/2994