Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-6929067

Stack guard pages should be removed when thread is detached

    XMLWordPrintable

Details

    • Bug
    • Resolution: Fixed
    • P2
    • hs18
    • hs18
    • hotspot
    • None
    • b02
    • x86
    • solaris_10

    Backports

      Description

        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?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 :-)

        > > 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
        > >

        Attachments

          Issue Links

            Activity

              People

                coleenp Coleen Phillimore
                kvn Vladimir Kozlov
                Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                  Created:
                  Updated:
                  Resolved:
                  Imported:
                  Indexed: