[9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault

[Chris Plummer]

Sorry about the long delay in getting back to this. I ran into two 
separate JPRT issues that were preventing me from testing these changes, 
plus I was on vacation last week. Here's an updated webrev. I'm not sure 
where we left things, so I'll just say what's changed since the original 
version:

1. Rewrote the test to be in Java instead of a shell script.
2. Moved the test from hotspot/test/runtime/memory to 
jdk/test/tools/launcher
3. Added STACK_SIZE_MINIMUM to java.c, allowing a makefile to override 
the default 32k minimum value.

https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.02/

thanks,

Chris

On 11/19/14 7:52 AM, Chris Plummer wrote:
> On 11/19/14 2:12 AM, David Holmes wrote:
>> On 19/11/2014 6:49 PM, Chris Plummer wrote:
>>> I've update the webrev to add STACK_SIZE_MINIMUM in place of the 32k
>>> references, and also moved the test from hotspot/test/runtime to
>>> jdk/test/tools/launcher as David requested. That required some
>>> adjustments to the test script, since test_env.sh does not exist in
>>> jdk/test, so I had to pull in the bits I needed into the script.
>>
>> Is there a reason this needs a shell script instead of using the 
>> testlibrary tools to launch the VM and check the output?
> Not that I'm aware of. I guess I just really didn't look at what it 
> would take to make it all in java. I'll have a look at java examples 
> and convert it.
>
> Chris
>>
>> Sorry that should have been mentioned much earlier.
>>
>> David
>>
>>
>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.01/
>>>
>>> I still need to rerun through JPRT. I'll do so once there are no more
>>> suggested changes.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 11/18/14 2:08 PM, Chris Plummer wrote:
>>>> Adding core-libs-dev at openjdk.java.net, since one of the changes is in
>>>> java.c.
>>>>
>>>> Chris
>>>>
>>>> On 11/12/14 6:43 PM, David Holmes wrote:
>>>>> Hi Chris,
>>>>>
>>>>> Sorry for the delay.
>>>>>
>>>>> On 13/11/2014 5:44 AM, Chris Plummer wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I'm still looking for reviewers.
>>>>>
>>>>> As the change is to the launcher it needs to be reviewed by the
>>>>> launcher owner - which I think is serviceability (though also cc'd
>>>>> Kumar :) ).
>>>>>
>>>>> Launcher change, and your rationale, seems okay to me. I'd probably
>>>>> put the test in to jdk/test/tools/launcher/ though.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 11/7/14 7:53 PM, Chris Plummer wrote:
>>>>>>> This is an initial review for 6762191. I'm guessing there will be
>>>>>>> recommendations to fix in a different way, but thought this 
>>>>>>> would be a
>>>>>>> good time to start the discussion.
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6762191
>>>>>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.jdk/
>>>>>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.hotspot/
>>>>>>>
>>>>>>> The bug is that if the -Xss size is set to something very small 
>>>>>>> (like
>>>>>>> 16k), on linux there will be a crash due to overwriting the end 
>>>>>>> of the
>>>>>>> stack. This happens before hotspot can compute its stack needs and
>>>>>>> verify that the stack is big enough.
>>>>>>>
>>>>>>> It didn't seem viable to move the hotspot stack size check 
>>>>>>> earlier. It
>>>>>>> depends on too much other work done before that point, and the 
>>>>>>> changes
>>>>>>> would have been disruptive. The stack size check is currently 
>>>>>>> done in
>>>>>>> os::init_2().
>>>>>>>
>>>>>>> What is needed is a check before the thread is created. That way we
>>>>>>> can create a thread with a big enough stack to handle all needs 
>>>>>>> up to
>>>>>>> the point of the check in os::init_2(). This initial check does not
>>>>>>> need to be the final check. It just needs to confirm that we have
>>>>>>> enough stack to get us to the check in os::init_2().
>>>>>>>
>>>>>>> I decided to check in java.c if the -Xss size is too small, and 
>>>>>>> set it
>>>>>>> to a larger size if it is. I hard coded this size to 32k (I'll 
>>>>>>> explain
>>>>>>> why 32k later). I suspect this is the part that will result in some
>>>>>>> debate. If you have better suggestions let me know. If it does stay
>>>>>>> here, then probably the 32k needs to be a #define, and maybe 
>>>>>>> even an
>>>>>>> OS porting interface, but I'm not sure where to put it.
>>>>>>>
>>>>>>> The reason I chose 32k is because this is big enough for all 
>>>>>>> platforms
>>>>>>> to get to the stack size check in os::init_2(). It is also smaller
>>>>>>> than the actual minimum stack size allowed on any platform. 32-bit
>>>>>>> windows has the smallest requirement at 64k. I add some printfs to
>>>>>>> print the minimum stack requirement, and then ran a simple JTReg 
>>>>>>> test
>>>>>>> with every JPRT supported platform to get the results.
>>>>>>>
>>>>>>> The TooSmallStackSize.sh will run "java -version" with -Xss16k,
>>>>>>> -Xss32k, and -XXss<minsize>, where <minsize> is the size from the
>>>>>>> error message produced by the JVM, such as in the following:
>>>>>>>
>>>>>>> $ java -Xss32k -version
>>>>>>> The stack size specified is too small, Specify at least 100k
>>>>>>> Error: Could not create the Java Virtual Machine.
>>>>>>> Error: A fatal exception has occurred. Program will exit.
>>>>>>>
>>>>>>> I ran this test through JPRT on all platforms, and they all pass.
>>>>>>>
>>>>>>> One thing to point out is that Windows behaves a bit different than
>>>>>>> the other platforms. It always rounds the stack size up to a 
>>>>>>> multiple
>>>>>>> of 64k , so even if you specify -Xss16k, you get a 64k stack. On
>>>>>>> 32-bit Windows with C1, 64k is also the minimum requirement, so 
>>>>>>> there
>>>>>>> is no error produced in this case. However, on 32-bit Windows 
>>>>>>> with C2,
>>>>>>> 68k is the minimum, so an error is produced since the stack will 
>>>>>>> only
>>>>>>> be 64k. There is no bug here. It's just a bit confusing.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>
>>>>
>>>
>