Request for review: 6761744 Hotspot crashes if process size limit is exceeded

Tao Mao tao.mao at oracle.com
Wed May 1 17:36:35 UTC 2013


I tried your reproducer on 7u21 and it had no complaint.

jdk/1.7.0_21/bin/java -XX:+UseParallelGC -Xmx4092M -version

java version "1.7.0_21-ea"
Java(TM) SE Runtime Environment (build 1.7.0_21-ea-b01)
Java HotSpot(TM) Server VM (build 23.7-b01, mixed mode)

Note that 4092M < 4G.

What was the symptom you got? How large is the tested machine's memory?

Thanks.
Tao


On 5/1/13 2:30 AM, Vladimir Kempik wrote:
> ParallelScavange and 7u21.
>
> What was happening - gc, when allocating memory, was overwriting some 
> of it's own code and then calling bunch of zeroes.
>
> So I think safe memory increase and overflow check, from the patch, 
> fixed the problem.
>
> Vladimir
>
>
>
> 01.05.2013, в 1:52, Tao Mao <tao.mao at oracle.com 
> <mailto:tao.mao at oracle.com>> написал(а):
>
>> IMHO, what the customer reported could be a similar but different bug.
>>
>> What version did they use, and what gc?
>>
>> Thanks.
>> Tao
>>
>> On 4/30/13 1:22 PM, Vladimir Kempik wrote:
>>> Hi
>>>
>>> It has easy reproducer.
>>>
>>> In solaris sparc, run 32-bit java
>>>
>>> java -Xmx4092M -version
>>>
>>> Java6/7 crashes. With patch from webrev.03 it works as it should, 
>>> saying it has not enough memory .
>>>  Vladimir
>>>
>>> 30.04.2013, в 22:11, Tao Mao <tao.mao at oracle.com 
>>> <mailto:tao.mao at oracle.com>> написал(а):
>>>
>>>> Hi Vladimir,
>>>>
>>>> Can you point me to your bug you think it's associated with?
>>>>
>>>> Thanks.
>>>> Tao
>>>>
>>>> On 4/30/13 3:33 AM, Vladimir Kempik wrote:
>>>>> Hello.
>>>>>
>>>>> I have a customer with non-escalated bug that is duplicate of this 
>>>>> bug. I checked with patch from webrev.03 and can confirm that 
>>>>> fixed the bug.
>>>>>
>>>>> Could you guys approve it if it's fine?
>>>>>
>>>>> Thanks. Vladimir.
>>>>> On 24.04.2013 22:35, Tao Mao wrote:
>>>>>> For 32-bit builds: The current changeset provides the first 
>>>>>> "protection" of heap size handling. Then comes handling whether 
>>>>>> we can allocate a certain amount of a heap.
>>>>>>
>>>>>> For 64-bit builds: a machine with 512M-1GB memory should fail in 
>>>>>> many other tests as well, due to inability to lauch jvm.
>>>>>>
>>>>>> Anyway, I ran the test and it passed.
>>>>>>
>>>>>> --------------------------------------------------
>>>>>> Simply,
>>>>>>
>>>>>> -bash-4.1$ ulimit -v 134217728 (128m for human reading)
>>>>>> -bash-4.1$ ulimit -m 134217728 (128m FHR)
>>>>>>
>>>>>> (cannot limit virtual memory to zero or set vm.swappiness=0, in 
>>>>>> which case the server would hang immediately w/o even launching 
>>>>>> jvm. But, the setting should satisfy your question.)
>>>>>>
>>>>>> Then, run jtreg. Then, pass.
>>>>>> --------------------------------------------------
>>>>>>
>>>>>> BTW, since you've jumped in this thread ^_^ can you check the way 
>>>>>> I get system property through "test.java.opts" to set inside jvm 
>>>>>> process? It's in 
>>>>>> test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java
>>>>>>
>>>>>> Thank you.
>>>>>> Tao
>>>>>>
>>>>>> On 4/24/13 3:36 AM, Leonid Mesnik wrote:
>>>>>>> On 04/22/2013 10:07 PM, Tao Mao wrote:
>>>>>>>> This should be handled by a separate CR (probably by this CR 
>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-7112912)
>>>>>>>>
>>>>>>> Why this? I mean should your test pass on hosts with *small* 
>>>>>>> amount of memory.
>>>>>>>
>>>>>>> Leonid
>>>>>>>> Thanks.
>>>>>>>> Tao
>>>>>>>>
>>>>>>>> On 4/22/13 2:46 AM, Leonid Mesnik wrote:
>>>>>>>>> Tao
>>>>>>>>>
>>>>>>>>> This is not review, just question.
>>>>>>>>>
>>>>>>>>> Should you test pass on hosts with 512M-1GB memory and no swap?
>>>>>>>>>
>>>>>>>>> Leonid
>>>>>>>>> On 04/19/2013 09:27 PM, Tao Mao wrote:
>>>>>>>>>> 1. check overflow routine is wrapped up.
>>>>>>>>>>
>>>>>>>>>> 2. The reduction of code duplication is adopted.
>>>>>>>>>>
>>>>>>>>>> 3. Develop a way to intake 0~multiple external vm options in 
>>>>>>>>>> jtreg main(). Hope it helps others implement similar 
>>>>>>>>>> functionality.
>>>>>>>>>>
>>>>>>>>>> webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.02/
>>>>>>>>>>
>>>>>>>>>> test:
>>>>>>>>>> JTREG: passed.
>>>>>>>>>> jtreg -jdk:/home/tamao/jdk1.7.0_04-i586 -vmoptions:"-tamao 
>>>>>>>>>> <GC_OPTION>" 
>>>>>>>>>> /home/tamao/src/6761744CrashIfProcessSizeLimitExceeded_hsx24/test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java
>>>>>>>>>>
>>>>>>>>>> where GC_OPTION rotates in -XX:+UseParallelGC -XX:+UseG1GC 
>>>>>>>>>> -XX:+UseSerialGC -XX:+UseParNewGC -XX:+UseConcMarkSweepGC
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>> Tao
>>>>>>>>>>
>>>>>>>>>> On 4/15/13 1:37 AM, Bengt Rutisson wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Tao,
>>>>>>>>>>>
>>>>>>>>>>> I agree with John. The changes look fine and I'm ok with 
>>>>>>>>>>> them. But it would look nicer to add the 
>>>>>>>>>>> CollectedHeap::add_and_verify_no_overflow() method.
>>>>>>>>>>>
>>>>>>>>>>> Also, regarding the test, a couple of minor things.
>>>>>>>>>>>
>>>>>>>>>>> First, with the new naming convention I think the test 
>>>>>>>>>>> should be called something like:
>>>>>>>>>>>
>>>>>>>>>>> test/gc/init/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>>>>>>>>>>>
>>>>>>>>>>> Then I think you can reduce the code duplication a bit. You 
>>>>>>>>>>> create the same ProcessBuilder and OutputAnalyzer in both 32 
>>>>>>>>>>> and 64 bit cases. So maybe it could look like this instead:
>>>>>>>>>>>
>>>>>>>>>>> public class TestHandleExceedingProcessSizeLimitOn32BitSystems {
>>>>>>>>>>>   public static void main(String args[]) throws Exception {
>>>>>>>>>>>     ProcessBuilder pb =
>>>>>>>>>>>       
>>>>>>>>>>> ProcessTools.createJavaProcessBuilder(System.getProperty("test.vm.opts"),
>>>>>>>>>>>                                               "-Xmx3072m",
>>>>>>>>>>>                                               
>>>>>>>>>>> "-XX:MaxPermSize=1024m",
>>>>>>>>>>>                                               "-version");
>>>>>>>>>>>     OutputAnalyzer output = new OutputAnalyzer(pb.start());
>>>>>>>>>>>
>>>>>>>>>>>     String dataModel = 
>>>>>>>>>>> System.getProperty("sun.arch.data.model");
>>>>>>>>>>>     if (dataModel.equals("32")) {
>>>>>>>>>>>       output.shouldContain("The size of the object heap + 
>>>>>>>>>>> perm gen exceeds the maximum representable size");
>>>>>>>>>>>       if (output.getExitValue() == 0) {
>>>>>>>>>>>         throw new RuntimeException("Not expected to get exit 
>>>>>>>>>>> value 0");
>>>>>>>>>>>       }
>>>>>>>>>>>     } else if (dataModel.equals("64")) {
>>>>>>>>>>>       output.shouldHaveExitValue(0);
>>>>>>>>>>>     }
>>>>>>>>>>>   }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> There is also a small bug in the test. If you run JTreg 
>>>>>>>>>>> without passing any vmoptions the test will fail. The reason 
>>>>>>>>>>> is that System.getProperty("test.vm.opts") will evaluate to 
>>>>>>>>>>> the empty string. This will be passed as the first argument 
>>>>>>>>>>> to the Java process, which will assume that this is the 
>>>>>>>>>>> class name. So, it fails to start because it can't load the 
>>>>>>>>>>> class <empty string>.
>>>>>>>>>>>
>>>>>>>>>>> To reproduce the problem use this command line:
>>>>>>>>>>>
>>>>>>>>>>> java  -jar <path_to_jtreg>/lib/jtreg.jar 
>>>>>>>>>>> test/gc/6761744/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>>>>>>>>>>>
>>>>>>>>>>> I guess the fix is to check that 
>>>>>>>>>>> System.getProperty("test.vm.opts") is not empty before 
>>>>>>>>>>> passing it on to createJavaProcessBuilder().
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Bengt
>>>>>>>>>>>
>>>>>>>>>>> On 4/13/13 3:15 AM, John Cuthbertson wrote:
>>>>>>>>>>>> Hi Tao,
>>>>>>>>>>>>
>>>>>>>>>>>> This looks OK to me.
>>>>>>>>>>>>
>>>>>>>>>>>> I can see what Thomas is saying though. All of the checks 
>>>>>>>>>>>> involve something like:
>>>>>>>>>>>>
>>>>>>>>>>>> total += some_value;
>>>>>>>>>>>> if (total < some_value) {
>>>>>>>>>>>>   // We must have overflowed
>>>>>>>>>>>>   vm_exit(...);
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> So a function in CollectedHeap (the base class of 
>>>>>>>>>>>> SharedHeap and ParallelScavengeHeap) might make sense. For 
>>>>>>>>>>>> example:
>>>>>>>>>>>>
>>>>>>>>>>>> total_reserved = 0;
>>>>>>>>>>>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>>>>>>>>>>>> max_heap_size);
>>>>>>>>>>>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>>>>>>>>>>>> max_perm_size);
>>>>>>>>>>>>
>>>>>>>>>>>> Where the function is:
>>>>>>>>>>>>
>>>>>>>>>>>> size_t add_and_verify_no_overflow(size_t x, size_t y) {
>>>>>>>>>>>>   const char* msg = "...";
>>>>>>>>>>>>   x += y;
>>>>>>>>>>>>   if (x < y) {
>>>>>>>>>>>>     vm_exit(msg);
>>>>>>>>>>>>   }
>>>>>>>>>>>>   return x;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> But I can live with your current changes.
>>>>>>>>>>>>
>>>>>>>>>>>> JohnC
>>>>>>>>>>>>
>>>>>>>>>>>> On 4/10/2013 9:52 AM, Tao Mao wrote:
>>>>>>>>>>>>> Hi Bengt,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thank you for reviewing. A new webrev is updated.
>>>>>>>>>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.01/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>> Tao
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 4/10/13 1:54 AM, Bengt Rutisson wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Tao,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This change looks good. Thanks for adding the JTReg test, 
>>>>>>>>>>>>>> it looks good!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> One minor nit:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp 
>>>>>>>>>>>>>> I would suggest to store "(size_t) 
>>>>>>>>>>>>>> align_size_up(pgs->max_size(), HeapRegion::GrainBytes)" 
>>>>>>>>>>>>>> in a local variable rather than duplicating the calculation.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Bengt
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 4/8/13 7:22 AM, Tao Mao wrote:
>>>>>>>>>>>>>>> 6761744 Hotspot crashes if process size limit is exceeded
>>>>>>>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6761744
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.00/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> changeset:
>>>>>>>>>>>>>>> The fix only needs to go to hsx24 since there's no perm 
>>>>>>>>>>>>>>> gen in hotspot-25. Thus, the webrev is based on hsx24 repo.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It provides for 32-bit builds a preventive check of the 
>>>>>>>>>>>>>>> size of "the object heap + perm gen" before reserving VM 
>>>>>>>>>>>>>>> memory. The total size should not exceed 4096MB (i.e. 
>>>>>>>>>>>>>>> 4GB) for 32-bit builds; otherwise, the total doesn't 
>>>>>>>>>>>>>>> make sense and, what's worse, overflow occurs. It will 
>>>>>>>>>>>>>>> consequentially trigger anther error of memory access 
>>>>>>>>>>>>>>> violation, which was not protected.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> jtreg testing java code is also written, checking both 
>>>>>>>>>>>>>>> 32-bit and (trivially) 64-bit builds.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> testing:
>>>>>>>>>>>>>>> check jtreg tests with flags -XX:+UseParallelGC, 
>>>>>>>>>>>>>>> -XX:+UseG1GC, -XX:+UseParNewGC, -XX:+UseConcMarkSweepGC, 
>>>>>>>>>>>>>>> -XX:+UseSerialGC and builds of 32-bit and 64-bit. All 
>>>>>>>>>>>>>>> passed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Needs JPRT test when pushing.
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> Leonid Mesnik
>>>>>>>>> JVM SQE
>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> Leonid Mesnik
>>>>>>> JVM SQE
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20130501/e0ddd94f/attachment.htm>


More information about the hotspot-gc-dev mailing list