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

Vladimir Kempik vladimir.kempik at oracle.com
Wed May 1 09:30:10 UTC 2013


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> написал(а):

> 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> написал(а):
>> 
>>> 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.org/pipermail/hotspot-gc-dev/attachments/20130501/6124041e/attachment.htm>


More information about the hotspot-gc-dev mailing list