Stack guard pages

[Vladimir Kozlov]

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 <aph at redhat.com> wrote:
>> On 02/23/2010 07:15 PM, Andrew John Hughes wrote:
>>> On 23 February 2010 16:16, Andrew Haley <aph at redhat.com> wrote:
>>>> I've been working on a bug in OpenOffice where using Java causes a
>>>> later crash.  https://bugzilla.novell.com/show_bug.cgi?id=578802
>>>>
>>>> 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
>>
> 
> 
>