JDK-8234372 : Investigate use of Thread::stack_base() and queries for "in stack"
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 14
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-11-19
  • Updated: 2020-10-07
  • Resolved: 2020-02-13
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 15
15 b11Fixed
Related Reports
Relates :  
JDK-8238988 - Rename thread "in stack" methods and add is_in_stack_range
Relates :  
JDK-8254189 - Improve comments for StackOverFlow and fix in_xxx() functions
Relates :  
JDK-8215355 - Object monitor deadlock with no threads holding the monitor (using jemalloc 5.1)
Description
In light of JDK-8215355 we should check all uses of Thread::stack_base() to watch for range tests that should be exclusive but are inclusive, and vice-versa. 

Review comments from JDK-8215355

On 18/11/2019 9:58 pm, Thomas St��fe wrote:
> There might be more cases like this, e.g.
>
> frame_x86.cpp  frame::is_interpreted_frame_valid():
>
> if (locals > thread->stack_base() || locals < (address) fp()) return false; 
>
> Many of them could use Thread::in_usable_stack(), I assume.

On 19/11/2019 3:36 am, Daniel D. Daugherty wrote:
> src/hotspot/share/runtime/thread.cpp
> 
>      Not your issue, but these two lines feel strange/wrong:
> 
>          L1008:   // Allow non Java threads to call this without stack_base
>          L1009:   if (_stack_base == NULL) return true;
> 
>      When _stack_base is NULL, any 'adr' is in the caller's stack? The
>      comment is not helping understand why this is so...
> 
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
> 
>      Again, not your issue, but these four lines are questionable:
> 
>          L383     Address sp      = lastSPDbg();
>          L384     Address stackBase = getStackBase();
>          L385     // Be robust
>          L386     if (sp == null) return false;
> 
>      I can see why a NULL sp would cause a "false" return since obviously
>      something is a amiss in the frame. However, the C++ code doesn't make
>      this check so why does the SA code?
> 
>      And this code doesn't check stackBase == NULL so it's not matching
>      the C++ code either.

> So we have Thread::is_in_usable_stack(), Thread::on_local_stack() and
> Thread::is_in_stack()? I haven't compared all three side by side, but
> there might be some cleanup work that can be done here (in a different
> bug).
Comments
Thread::is_in_usable_stack is only called for JavaThreads, so should be a JavaThread method. If used on non-JavaThreads it would incorrectly exclude non-existent guard pages from the check. I think some renaming is needed here: Thread::is_in_stack -> Thread::is_in_live_stack Thread::on_local_stack -> Thread::is_in_full_stack // I wanted to use "mapped" to imply full stack segment but "mapped" has a special meaning at the OS page level Thread::is_in_usable_stack -> JavaThread::is_in_usable_stack UPDATE: renaming deferred to JDK-8238988 except for moving is_in_usable_stack to JavaThread.
13-02-2020
URL: https://hg.openjdk.java.net/jdk/jdk/rev/e795d446f21a User: dholmes Date: 2020-02-13 01:36:10 +0000
13-02-2020
We can also make the following simpification in os_linux.cpp and os_windows.cpp: - if (addr > thread->stack_reserved_zone_base() && addr < thread->stack_base()) { + if (thread->is_in_usable_stack(addr)) {
11-02-2020
Looking at all comparisons Thread::stack_base() these ones, using >=, seemed possibly incorrect (as stack_base() is the first/last address NOT in the stack): ./cpu/aarch64/frame_aarch64.cpp: if ((address)sender_sp >= thread->stack_base()) { ./cpu/arm/frame_arm.cpp: if ((address)sender_sp >= thread->stack_base()) { ./cpu/x86/frame_x86.cpp: if ((address)sender_sp >= thread->stack_base()) { But these are actually correct as they are negative tests: // Is sender_sp safe? if ((address)sender_sp >= thread->stack_base()) { return false; } ./os_cpu/linux_s390/thread_linux_s390.cpp: if (stack_base() >= (address)istate && (address)istate > stack_end()) { return false; } It is a little unclear what exactly this code is checking, as this is also a negative test, but the >= does seem incorrect. In fact this looks like it should be replaced with on_local_stack(istate). ./os_cpu/solaris_x86/thread_solaris_x86.cpp: if ((address)ret_fp >= jt->stack_base() || ret_fp < ret_sp) { ret_fp = NULL; } This is a negative test and so is also correct. These ones using <= are incorrect: ./cpu/arm/frame_arm.cpp: (sp <= thread->stack_base()) && ./cpu/arm/frame_arm.cpp: (unextended_sp <= thread->stack_base()) && ./cpu/arm/frame_arm.cpp: (fp <= thread->stack_base()) && ./cpu/arm/frame_arm.cpp: bool saved_fp_safe = ((address)saved_fp <= thread->stack_base()) && (saved_fp > sender_sp); ./cpu/arm/frame_arm.cpp: bool saved_fp_safe = ((address)saved_fp <= thread->stack_base()) && (saved_fp >= sender_sp); ./cpu/arm/frame_arm.cpp: bool jcw_safe = (jcw <= thread->stack_base()) && (jcw > (address)sender.fp()); ./cpu/ppc/frame_ppc.cpp: bool fp_safe = (fp <= thread->stack_base()) && (fp > sp); ./cpu/ppc/frame_ppc.cpp: bool fp_interp_safe = (fp <= thread->stack_base()) && (fp > sp) && ./cpu/ppc/frame_ppc.cpp: if (sender_fp > thread->stack_base() || sender_fp <= fp) { ./cpu/s390/frame_s390.cpp: bool fp_safe = (fp <= thread->stack_base()) && (fp > sp); ./cpu/s390/frame_s390.cpp: bool fp_interp_safe = (fp <= thread->stack_base()) && (fp > sp) && ./cpu/s390/frame_s390.cpp: if (sender_fp > thread->stack_base() || sender_fp <= fp) { ./cpu/sparc/frame_sparc.cpp: bool sp_safe = (_SP <= thread->stack_base()) && ./cpu/sparc/frame_sparc.cpp: bool unextended_sp_safe = (_UNEXTENDED_SP <= thread->stack_base()) && ./cpu/sparc/frame_sparc.cpp: bool fp_safe = (_FP <= thread->stack_base()) && ./cpu/sparc/frame_sparc.cpp: bool sender_fp_safe = (sender_fp <= thread->stack_base()) && ./cpu/sparc/frame_sparc.cpp: bool jcw_safe = (jcw <= thread->stack_base()) && (jcw > sender_fp); Looks like we only have a correct frame::safe_for_sender on x86 and Aarch64! There are also negative checks for "locals > thread->stack_base()" that should be using >=.
11-02-2020
In relation to: > frame_x86.cpp frame::is_interpreted_frame_valid(): > > if (locals > thread->stack_base() || locals < (address) fp()) return false; > > Many of them could use Thread::in_usable_stack(), I assume. This case is looking at the live portion of the stack and so could possibly be using is_in_stack(), but on closer inspection it seems the check above is more strict - it isn't just in the live stack but in a specific portion of the live stack. So I think it can remain as is. Though the initial clause should be checking >= thread->stack_base(). frame::safe_for_sender mostly duplicates the logic of Thread::is_in_usable_stack() but with a subtle difference that it does not exclude the "reserved zone" from the usable stack. It is unclear if this is actually an oversight or a deliberate choice. Looking at the history of Thread::is_in_usable_stack, it was added by: JDK-8016131: nsk/sysdict/vm/stress/chain tests crash the VM in 'entry_frame_is_first()' (2013-06-12 ) but at the time the guard size was simply the sum of StackRedPages and StackYellowPages. The reserved pages were added to the calculation as part of: JDK-8046936: JEP 270: Reserved Stack Areas for Critical Sections (2015-12-11) The safe_for_sender code added the stack guard to its calculation under JDK-8005849: JEP 167: Event-Based JVM Tracing (2013-06-10) I changed the code to use is_in_usable_stack() and it passes tier 1-3. [~fparain] Do you think it was simply an oversight that the JEP 270 work did not also update the stack guard calculations in frame::safe_for_sender? UPDATE: PPC and S390 versions of this code do include the reserved pages.
11-02-2020
[~dholmes] The fact that JEP-270 did not include the reserved zone in the calculation of frame::safe_for_sender() looks like a bug to me. When the reserved zone is activated, it is expected that some interpreted code could use this page.
10-02-2020
In relation to: > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java > > Again, not your issue, but these four lines are questionable: > > L383 Address sp = lastSPDbg(); > L384 Address stackBase = getStackBase(); > L385 // Be robust > L386 if (sp == null) return false; > > I can see why a NULL sp would cause a "false" return since obviously > something is a amiss in the frame. However, the C++ code doesn't make > this check so why does the SA code? Arguably the SA code is being more robust because something may have gone wrong in getting the information from the original thread and carrying it over to the SA. However, that would argue for even more checks that don't seem to be present. Regardless, in the absence of any indication that this is broken or otherwise a bad thing to do, I do not propose to change it, just to err on the side of caution. > And this code doesn't check stackBase == NULL so it's not matching > the C++ code either. The C++ code claims to only check for NULL to allow for a non-JavaThread. The SA code is only for JavaThreads, hence no NULL check is needed to match that. That aside the only time a NULL stack base can be encountered is if querying a Thread that has not yet started execution, and such threads should not be visible to any third-party observers - whether JavaThreads or non-JavaThreads. For the C++ Thread::is_in_stack, which is only executed by the current thread, it is impossible to find a NULL _stack_base unless this is a thread that was started from an initial entry point other than thread_native_entry - as the very first thing thread_native_entry does is set up the stack base and size. Consequently the NULL check in the C++ code seems superfluous - something which testing seems to confirm - ref [~coleenp]. Also note stack_base() already asserts _stack_base is not NULL.
10-02-2020
In FrameValues::print we have void FrameValues::print(JavaThread* thread) { ... intptr_t* v0 = _values.at(min_index).location; intptr_t* v1 = _values.at(max_index).location; if (thread == Thread::current()) { while (!thread->is_in_stack((address)v0)) { v0 = _values.at(++min_index).location; } while (!thread->is_in_stack((address)v1)) { v1 = _values.at(--max_index).location; } } else { while (!thread->on_local_stack((address)v0)) { v0 = _values.at(++min_index).location; } while (!thread->on_local_stack((address)v1)) { v1 = _values.at(--max_index).location; } } If we only check the live stack of the current thread then it seems to me we likely only need to check the usable stack of any other thread, rather than the full stack. Though it may not make any difference depending on exactly how this code is walking an address range - as the unusable part of the stack can only be reached after we've encountered the usable stack.
10-02-2020
In relation to: > Not your issue, but these two lines [in Thread::is_in_stack] feel strange/wrong: > > L1008: // Allow non Java threads to call this without stack_base > L1009: if (_stack_base == NULL) return true; > > When _stack_base is NULL, any 'adr' is in the caller's stack? The > comment is not helping understand why this is so... That code was added by: JDK-6964458: Reimplement class meta-data storage to use native memory which was the PermGen removal work. This is used in the meta-data handle implementation, but it is not at all clear why the handles are required to be on the stack of a JavaThread but not of a non-JavaThread. [~coleenp] do you have any idea here?
07-02-2020
This method is usable only by the current thread and checks if an address if within the used part of the stack: bool Thread::is_in_stack(address adr) const { assert(Thread::current() == this, "is_in_stack can only be called from current thread"); address end = os::current_stack_pointer(); // Allow non Java threads to call this without stack_base if (_stack_base == NULL) return true; if (stack_base() > adr && adr >= end) return true; return false; } This method can be called on any thread and checks if the address is within the usable portion of the stack that has been allocated to this thread i.e. it ignores the guard regions that the JVM adds: bool Thread::is_in_usable_stack(address adr) const { size_t stack_guard_size = os::uses_stack_guard_pages() ? JavaThread::stack_guard_zone_size() : 0; size_t usable_stack_size = _stack_size - stack_guard_size; return ((adr < stack_base()) && (adr >= stack_base() - usable_stack_size)); } This method can be called on any thread and checks if the address is anywhere within the the stack that has been allocated to this thread: bool on_local_stack(address adr) const { // QQQ this has knowledge of direction, ought to be a stack method return (_stack_base > adr && adr >= stack_end()); } Each of these methods is used for different usecases: - is_in_stack has a few uses in relation to frames, metadata handles and unhandled oop checking, where we are looking at the live stack of the current thread - on_local_stack is used with error detection looking for stackoverflows etc and so needs to consider the entire allocated stack. - is_in_usable_stack only has two uses both of which raise questions: In frame.cpp: JavaCallWrapper* frame::entry_frame_call_wrapper_if_safe(JavaThread* thread) const { JavaCallWrapper** jcw = entry_frame_call_wrapper_addr(); address addr = (address) jcw; // addr must be within the usable part of the stack if (thread->is_in_usable_stack(addr)) { return *jcw; } return NULL; } It seems reasonable that we check the address is in the usable stack, but that seems insufficient as it only checks that it starts in the usable stack, not that it is contained in the usable stack. So possibly this check should be enhanced. In share/jfr/leakprofiler/checkpoint/rootResolver.cpp bool ReferenceToThreadRootClosure::do_thread_stack_fast(JavaThread* jt) { assert(jt != NULL, "invariant"); assert(!complete(), "invariant"); if (_callback.entries() == 0) { _complete = true; return true; } RootCallbackInfo info; info._high = NULL; info._low = NULL; info._context = jt; info._system = OldObjectRoot::_threads; info._type = OldObjectRoot::_stack_variable; for (int i = 0; i < _callback.entries(); ++i) { const address adr = _callback.at(i).addr<address>(); if (jt->is_in_usable_stack(adr)) { info._high = adr; _complete = _callback.process(info); if (_complete) { return true; } } } assert(!complete(), "invariant"); return false; } This is a fast check and simply looks for an address in the usable stack, even though it may be outside the live part of the stack. Presumably any candidate locations are further screened somehow in case they are just junk values ??
07-02-2020
It might be if you take it out it crashes (or not!) and the stack in the crash will be your answer. I don't remember but it's likely this is how that line got in there.
07-02-2020