RFR (S) JDK-7107135 - Stack guard pages becomes writable

Fri Feb 22 01:36:58 PST 2013

Thank you Ioi, the webrev and the bug report are now visible.

Regarding the the copyright notice - you don't have to remove the SAP
copyright notice (and you probably aren’t allowed to). The line you
mention refers to the text below the line itself. That's because that
lines get replaced automatically if the sources are shipped to
licensees  (i.e. GPL replaced by Oracle commercial license). If you
grep trough the tests directory you'll find a bunch of other files
with SAP copyright (e.g.
test/runtime/7100935/TestConjointAtomicArraycopy.java). The JDK
contains even more examples (e.g. grep for IBM in
jdk/src/share/classes/java/awt/font).

So please put the copyright notices back into the corresponding files
(give credit where credit is due :)

Regards,
Volker

On Thu, Feb 21, 2013 at 7:28 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> Hi Vokler & Goetz,
>
> I have posted the review at:
>     http://cr.openjdk.java.net/~iklam/7107135/stack_guard_001/
>
> The bug should be
>     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7107135
>
> However, for some reason it's not visible to outside. Could someone at
> Oracle tell me how to fix this?
>
> Also, as in the webview, I have removed the SAP copyright notice (sorry!) as
> somewhere in the Oracle header it says "DO NOT ALTER .... THIS FILE HEADER".
> What should we do with code contributed from outside of Oracle.
>
> Thanks
> - Ioi
>
>
>
> On 02/21/2013 01:03 AM, Volker Simonis wrote:
>>
>> Hi Ioi,
>>
>> could you please always post webrev and bug URLs which are visible
>> from non-Oracle employees as well.
>>
>> I think the original bug together with a fix was submitted by a
>> colleague, but with the information you give it is hard to verify
>> this.
>>
>> Regards,
>> Volker
>>
>> On Wed, Feb 20, 2013 at 7:40 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>> [Moving the discussion to hotspot-runtime-dev at openjdk.java.net]
>>>
>>> The original request for review is cut-and-pasted atthe end of this
>>> message.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 02/20/2013 08:25 AM, Dean Long wrote:
>>>>
>>>> On 2/20/2013 3:01 AM, David Holmes wrote:
>>>>>
>>>>> On 20/02/2013 6:32 PM, Dean Long wrote:
>>>>>>
>>>>>> On 2/20/2013 12:12 AM, David Holmes wrote:
>>>>>>>
>>>>>>> On 20/02/2013 3:29 PM, Dean Long wrote:
>>>>>>>>
>>>>>>>> On 2/19/2013 6:48 PM, David Holmes wrote:
>>>>>>>>>
>>>>>>>>> On 20/02/2013 11:39 AM, Dean Long wrote:
>>>>>>>>>>
>>>>>>>>>> On 2/19/2013 5:35 PM, David Holmes wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 20/02/2013 11:24 AM, Dean Long wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> If os::Linux::default_guard_size() returns non-zero, we set an
>>>>>>>>>>>> attribute
>>>>>>>>>>>> for pthread_create telling it to create guard pages for us.
>>>>>>>>>>>> These guard pages aren't set to rwx by the dynamic linker
>>>>>>>>>>>> because
>>>>>>>>>>>> pthreads knows about them.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Are you saying that these guard pages don't get reset when the
>>>>>>>>>>> library
>>>>>>>>>>> is loaded (or that they will at least get repaired afterwards) ?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> They will not be reset.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Can you point me to any docs or the source code that demonstrates
>>>>>>>>> this? From previous discussions the OS only allowed you to set page
>>>>>>>>> bits not read them, and hence not add them. So I'd like to see how
>>>>>>>>> they are achieving this.
>>>>>>>>>
>>>>>>>> The magic happens here in change_stack_perm():
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> http://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/allocatestack.c#l314
>>>>>>>>
>>>>>>> But that simply makes the stack readable, writable and executable but
>>>>>>> skipping over the guard pages. The initial protection of the guard
>>>>>>> pages happens here:
>>>>>>>
>>>>>>>   634           if (mprotect (guard, guardsize, PROT_NONE) != 0)
>>>>>>>
>>>>>>> but I don't see anything in this code that prevents the guard pages
>>>>>>> from being modified when a library with executable stack is loaded?
>>>>>>>
>>>>>> The code path that does the modifying looks like: dlopen --> ... -->
>>>>>> __make_stacks_executable --> change_stack_perm.
>>>>>
>>>>>
>>>>> Ah I see. The above is what causes the problem, but it skips the
>>>>> built-in
>>>>> guard pages.
>>>>>
>>>>> I'm sure someone thought this made sense at some point. :( But it sure
>>>>> seems like a bug to me.
>>>>>
>>>> Which part seems buggy, changing the permissions?  It seems necessary as
>>>> long as executable stacks is
>>>> a supported feature.  It would be nice if dlopen() would just fail to
>>>> load
>>>> these libraries based on some flag
>>>> in the executable's ELF file.  I think the existing flag means "no
>>>> executable stacks used by this module",
>>>> but "no executable stacks used by this process" would make more sense
>>>> for
>>>> Java.
>>>>
>>>> dl
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>>> Also note that for user supplied stack memory there are no guard
>>>>>>> pages
>>>>>>> added.
>>>>>>>
>>>>>> OK.
>>>>>>
>>>>>> dl
>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> I wish there was a way to change the guardsize after the thread is
>>>>>>>> created.
>>>>>>>>
>>>>>>>> dl
>>>>>>>>
>>> Please review:
>>> http://javaweb.us.oracle.com/~iklam/webrev/7107135/stack_guard_001/
>>>
>>> Bug: Stack guard pages are no more protected after loading a shared
>>>       library with executable stack
>>> https://jbs.oracle.com/bugs/browse/JDK-7107135
>>>
>>> Background:
>>>
>>>      Recent versions of Linux support Non-Executable Stack
>>>      protection -- by default, the stack is made non-executable to
>>>      prevent code injection via overflowing on-stack buffers.
>>>
>>>      However, some old Linux DLLs require the stack to be
>>>      executable. For backwards compatibility, after loading such
>>>      DLLs, the Linux dynamic loader makes the stack executable.
>>>
>>>      Due to a limitation of the Linux system call API, the Linux
>>>      dynamic loader makes the stack readable/writable as well. This
>>>      disables Java's stack guard.
>>>
>>> Summary of fix:
>>>
>>>      1. Check if DLL requires executable stack by inspecting ELF header.
>>>      2. Enter a Safepoint and load such DLLs in the VM thread.
>>>         - immediately after loading, change all Java stack guards
>>>           back to PROT_NONE.
>>>      3. Leave Safepoint to resume Java execution.
>>>
>>>      I also added a global flag LoadExecStackDllInVMThread to load
>>>      such "bad" DLLs outside of the safepoint (in case the DLL
>>>      invokes JNI functions inside static constructors, which are
>>>      executed before dlopen() returns).
>>>
>>> Note:
>>>
>>>      I got this code from an outside contributor and I don't really
>>>      understand what this block does. Please comment if it's
>>>      correct:
>>>
>>>      1877:    ThreadInVMfromNative tiv(JavaThread::current());
>>>      1878:    debug_only(VMNativeEntryWrapper vew;)
>>>
>>> Tests executed:
>>>
>>>      * JPRT
>>>      * UTE (vm.quick.testlist)
>>>      * JTREG (hotspot/tests/runtime, hotspot/tests/closed/runtime)
>>>
>>> Thanks,
>>> Ioi
>>>
>