RFR: 8252921: NMT overwrite memory type for region assert when building dynamic archive [v3]

[Zhengyu Gu]

On Thu, 17 Sep 2020 01:23:32 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> > > > > This code in
> > > > > [virtualMemoryTracker.cpp](https://github.com/openjdk/jdk/blob/9a7dcdcdbad34e061a8988287fe691abfd4df305/src/hotspot/share/services/virtualMemoryTracker.cpp#L350)
> > > > > still looks buggy to me: ```
> > > > >     if (reserved_rgn->same_region(base_addr, size)) {
> > > > >       reserved_rgn->set_call_stack(stack);
> > > > >       reserved_rgn->set_flag(flag);
> > > > >       return true;
> > > > >     } else {
> > > > >       assert(reserved_rgn->overlap_region(base_addr, size), "Must be");
> > > > > 
> > > > >       // Overlapped reservation.
> > > > >       // It can happen when the regions are thread stacks, as JNI
> > > > >       // thread does not detach from VM before exits, and leads to
> > > > >       // leak JavaThread object
> > > > >       if (reserved_rgn->flag() == mtThreadStack) {
> > > > >         guarantee(!CheckJNICalls, "Attached JNI thread exited without being detached");
> > > > >         // Overwrite with new region
> > > > > ```
> > > > > 
> > > > > 
> > > > > Why is the "Overlapped reservation" fix not done when the two regions are exactly the same?
> > > > 
> > > > 
> > > > The same region branch was to deal with different scenario: recursive reservation, where os::reserve() ->
> > > > pd_reserve() -> os::reserve()
> > > > > If a JNI thread has exited without detaching, and its stack happens to be picked for a future memory reservation of the
> > > > > exact size, I think we will get the same assert as in this bug report.
> > > > 
> > > > 
> > > > Right, there is possibility. We can put the same guarantee there.
> > > 
> > > 
> > > I think the order of the checks should be changed:
> > > ```
> > > if (reserved_rgn->overlap_region(base_addr, size)) {
> > >     check for overlapped reservation
> > > }
> > > if (reserved_rgn->same_region(base_addr, size)) {
> > >    check for recursive reservation
> > > ```
> > > 
> > > 
> > > In fact, if the code had been written this way, this bug would not have happened.
> > 
> > 
> > Well, I don't know which bug you refer to, but overlap_region() covers same_region(), so reversing order is a no go.
> 
> I am talking the original assert reported by [JDK-8252921](https://bugs.openjdk.java.net/browse/JDK-8252921)
> 
> ```
> assert((flag() == mtNone || flag() == f)) failed: Overwrite memory type for region
> [0x000000a41cf40000-0x000000a41d040000), 3->23. ```
> 
> We get there because we have a old stack region which matches exactly as a newly reserved region. This case should also
> be handled by this code
> ```
> // Overlapped reservation.
> // It can happen when the regions are thread stacks, as JNI
> // thread does not detach from VM before exits, and leads to
> ```
> 
> The current code assumes that if you have an exact match, it must be from a recursive reveration. This is wrong.

In that case, it just hides the real problem.  Because the thread stack region can be remapped to any other types and
maybe different size.

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

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