FAQ
I filed bug

6929067: Stack guard pages should be removed when thread is detached

Vladimir

Andrew John Hughes wrote:
On 23 February 2010 19:50, Andrew Haley wrote:
On 02/23/2010 07:15 PM, Andrew John Hughes wrote:
On 23 February 2010 16:16, Andrew Haley wrote:
I've been working on a bug in OpenOffice where using Java causes a
later crash. https://bugzilla.novell.com/show_bug.cgi?idW8802

It's a very strange bug: Java is called, does something, and then the
thread is detached from the Java VM. Long after, a segfault occurs.

The reason for this crash is the way that stack guard pages work.
When DetachCurrentThread() calls
JavaThread::remove_stack_guard_pages(), the guard pages are not
removed. So, if at some point later in the process the stack grows
beyond the point where the guard pages were mapped, the Linux kernel
cannot expand the stack any further because the guard pages are in the
way, and a segfault occurs.

I've attached a reproducer for this bug to the end of this message.
It crashes on many of the Linux systems I've tried.

The right way to fix this is for remove_stack_guard_pages() to
munmap() the guard pages. However, it's essential not to split the
stack region, so if the stack has already grown beyond the guard
pages, we have to unmap() it. Like so:

bool os::create_stack_guard_pages(char* addr, size_t size) {
uintptr_t stack_extent, stack_base;
// get_stack_bounds() returns the memory extent mapped for the stack
// by the kernel.
if (get_stack_bounds(&stack_extent, &stack_base)) {
if (stack_extent < (uintptr_t)addr)
::munmap((void*)stack_extent, (uintptr_t)addr - stack_extent);
}

return os::commit_memory(addr, size);
}

bool os::remove_stack_guard_pages(char* addr, size_t size) {
return ::munmap(addr, size) == 0;
}

This fixes the bug. Unfortunately, there is no os:: interface for
create_stack_guard_pages() and remove_stack_guard_pages(), so this
solution doesn't fit well with the current organization of the VM. I
can't see any way of making this work only by touching os_linux.?pp .

I could simply patch OpenJDK locally for the Linux distros, but I
think we should have this discussion first.
Segfaults here with the latest OpenJDK7 b84:

$ /mnt/builder/jdk7/j2sdk-image/bin/java -version
openjdk version "1.7.0-internal"
OpenJDK Runtime Environment (build 1.7.0-internal-andrew_2010_02_23_18_45-b00)
OpenJDK 64-Bit Server VM (build 17.0-b09, mixed mode)

$ LD_LIBRARY_PATH=/mnt/builder/jdk7/j2sdk-image/jre/lib/amd64/server ./invoke
Hello
Segmentation fault

I think it would it would be clearer what the suggested fix entails if
you posted a diff or webrev of the suggested change.
Maybe, but I don't ATM have a suggested change. Everything I've tried is
rather intrusive, and potentially affects other targets.

However, here's what I've been testing, for information only.
Thanks. That helps a lot. I was confusing the functions you
introduce with the ones of the same name in
src/share/vm/runtime/thread.{c,h}pp

For the other platforms, would it not be sufficient for them to just
have simple implementations that call os::commti_memory and
os::uncommit_memory as before? The HotSpot developers can run the
change through JPRT to test it on all platforms.
Andrew.


--- ./src/share/vm/runtime/thread.cpp~ 2008-07-10 21:04:36.000000000 +0100
+++ ./src/share/vm/runtime/thread.cpp 2010-02-23 14:43:20.452135932 +0000
@@ -2075,7 +2075,7 @@
int allocate = os::allocate_stack_guard_pages();
// warning("Guarding at " PTR_FORMAT " for len " SIZE_FORMAT "\n", low_addr, len);

- if (allocate && !os::commit_memory((char *) low_addr, len)) {
+ if (allocate && !os::create_stack_guard_pages((char *) low_addr, len)) {
warning("Attempt to allocate stack guard pages failed.");
return;
}
@@ -2096,7 +2096,7 @@
size_t len = (StackYellowPages + StackRedPages) * os::vm_page_size();

if (os::allocate_stack_guard_pages()) {
- if (os::uncommit_memory((char *) low_addr, len)) {
+ if (os::remove_stack_guard_pages((char *) low_addr, len)) {
_stack_guard_state = stack_guard_unused;
} else {
warning("Attempt to deallocate stack guard pages failed.");
--- ./src/share/vm/runtime/os.hpp~ 2008-07-10 21:04:36.000000000 +0100
+++ ./src/share/vm/runtime/os.hpp 2010-02-23 14:49:27.512732036 +0000
@@ -168,6 +168,9 @@
static bool protect_memory(char* addr, size_t bytes);
static bool guard_memory(char* addr, size_t bytes);
static bool unguard_memory(char* addr, size_t bytes);
+ static bool create_stack_guard_pages(char* addr, size_t bytes);
+ static bool remove_stack_guard_pages(char* addr, size_t bytes);
+
static char* map_memory(int fd, const char* file_name, size_t file_offset,
char *addr, size_t bytes, bool read_only = false,
bool allow_exec = false);
--- ./src/os/linux/vm/os_linux.cpp~ 2010-02-23 11:42:40.005073691 +0000
+++ ./src/os/linux/vm/os_linux.cpp 2010-02-23 15:44:47.112387815 +0000
@@ -57,6 +57,10 @@
# include <sys/shm.h>
# include <link.h>

+#include <sstream>
+#include <iostream>
+#include <fstream>
+
#define MAX_PATH (2 * K)

// for timer info max values which include all bits
@@ -2292,6 +2296,53 @@
!= MAP_FAILED;
}

+static bool
+get_stack_bounds(uintptr_t *bottom, uintptr_t *top)
+{
+ using namespace std;
+
+ ostringstream oss;
+ oss << "/proc/" << syscall(SYS_gettid) << "/maps";
+ ifstream cin(oss.str().c_str());
+ while (!cin.eof())
+ {
+ string str;
+ getline(cin,str);
+ const string stack_str = "[stack]";
+ if (str.length() > stack_str.length()
+ && (str.compare(str.length() - stack_str.length(),
+ stack_str.length(), stack_str)
+ == 0))
+ {
+ istringstream iss(str);
+ iss.flags(ios::hex);
+ iss >> *bottom;
+ char c;
+ iss >> c;
+ iss >> *top;
+ uintptr_t sp = (intptr_t)__builtin_frame_address(0);
+ if (sp >= *bottom && sp <= *top)
+ return true;
+ }
+ }
+
+ return false;
+}
+
+bool os::create_stack_guard_pages(char* addr, size_t size) {
+ uintptr_t stack_extent, stack_base;
+ if (get_stack_bounds(&stack_extent, &stack_base)) {
+ if (stack_extent < (uintptr_t)addr)
+ ::munmap((void*)stack_extent, (uintptr_t)addr - stack_extent);
+ }
+
+ return os::commit_memory(addr, size);
+}
+
+bool os::remove_stack_guard_pages(char* addr, size_t size) {
+ return ::munmap(addr, size) == 0;
+}
+
static address _highest_vm_reserved_address = NULL;

// If 'fixed' is true, anon_mmap() will attempt to reserve anonymous memory

Search Discussions

Discussion Posts

Previous

Follow ups

Related Discussions

People

Translate

site design / logo © 2021 Grokbase