RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Tue Aug 12 14:53:50 UTC 2014
Hi Karen,
Thanks for the review!
Harold
On 8/12/2014 10:50 AM, Karen Kinnear wrote:
> Harold,
>
> Ship it! Looks nicer this way and covers all the cases I walked through manually.
> Thank you for so many rounds of careful work.
>
> thanks,
> Karen
>
> On Aug 12, 2014, at 8:47 AM, harold seigel <harold.seigel at oracle.com> wrote:
>
>> Hi,
>>
>> Please review yet another version of the fix for 8050485: http://cr.openjdk.java.net/~hseigel/bug_8050485_5/
>>
>> This version just saves the starting bytecode offsets, instead of starting and ending bytecode offsets, when a conditional branch is encountered.
>>
>> Thanks, Harold
>>
>> On 8/11/2014 2:45 PM, harold seigel wrote:
>>> Hi Karen,
>>>
>>> Thanks for the review! I updated the comments.
>>>
>>> I plan to use Dean's idea of just pushing the starting bytecode offset. Then there would be only one value to push and pop, not a pair.
>>>
>>> Thanks, Harold
>>>
>>> On 8/8/2014 6:27 PM, Karen Kinnear wrote:
>>>> Harold,
>>>>
>>>> Many thanks. Looks really good!
>>>>
>>>> 1. I like Keith's idea of push/pop pairs - if that works for you - or at least combine the 4 lines repeated
>>>> (end=, start=, assert, set_interval)
>>>> 2. ends_in_athrow comment in verifier.hpp and verifier.cpp 2333- (actually end in athrow or loop (i.e. don't end) right?)
>>>>
>>>> The cases I walked through all worked.
>>>>
>>>> thanks,
>>>> Karen
>>>> -
>>>> On Aug 8, 2014, at 9:03 AM, harold seigel wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Please review this latest version of the fix for 8050485. The implementation has been changed to handle forward and backward branches and catch clauses containing nested try blocks. It keeps a list of branch bytecode indexes that have already been parsed in order to detect when revisiting the same branch bytecode. This prevents infinite looping and ensures that all reachable bytecodes have been looked at.
>>>>>
>>>>> Updated webrev: http://cr.openjdk.java.net/~hseigel/bug_8050485_4/
>>>>>
>>>>> Thanks! Harold
>>>>>
>>>>> On 8/1/2014 1:20 PM, harold seigel wrote:
>>>>>> Hi Keith,
>>>>>>
>>>>>> I'm glad you are reviewing this fix. Here's some responses to your questions:
>>>>>>
>>>>>> 1. This type of behavior is allowed by the JVMS. See JVM Spec 8, section 4.10.1.4 <http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1.4>:
>>>>>>
>>>>>> A special form of frame assignability is allowed for an exception
>>>>>> handler of an |<init>| method. If the method invokes another
>>>>>> |<init>| method, and the invocation throws an exception, then the
>>>>>> current object is broken; nonetheless, a handler's target is type
>>>>>> safe iff all local variables in the handler's incoming type state
>>>>>> are |top|. This ensures that the handler's frame does not depend on
>>>>>> the current type.
>>>>>>
>>>>>> The above text was added as part of the fix for JDK-7020118 <https://bugs.openjdk.java.net/browse/JDK-7020118>.
>>>>>>
>>>>>> 2. The super() call site does result in 'this' being marked as initialized. I'll have to think some more about the scenario that you propose.
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>>
>>>>>> On 8/1/2014 12:48 PM, Keith McGuigan wrote:
>>>>>>> Hi Harold,
>>>>>>>
>>>>>>> Sorry for coming into this late and perhaps without full context, and I apologize if you've already answered this before, but I have a couple fundamental questions:
>>>>>>>
>>>>>>> 1. Is this type of behavior allowed by the JVMS? I seem to remember constraints in there that combined to flat-out prevented making a super() call be able to be protected by an exception handler... something about not being able to represent the uninit this type in the stack maps maybe. I could be mis-remembering here, or maybe have the rules been relaxed?
>>>>>>>
>>>>>>> 2. I assume that the super() call site still results in 'this' being marked as initialized, right? With your change, what's to stop someone from writing code that will stash the 'this' object into a static field, or stick it into the thrown exception before the inevitable throw occurs? If you can do this, the uninit object can still leak out of the method.
>>>>>>>
>>>>>>> --
>>>>>>> - Keith
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Aug 1, 2014 at 11:38 AM, harold seigel <harold.seigel at oracle.com <mailto:harold.seigel at oracle.com>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this latest webrev for fixing 8050485. The webrev
>>>>>>> is at: http://cr.openjdk.java.net/~hseigel/bug_8050485_3/
>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8050485_3/>
>>>>>>>
>>>>>>> This new webrev has two major changes. The first change is for
>>>>>>> branches. For both conditional forward and unconditional forward
>>>>>>> branches, the implementation now parses both paths. Because the
>>>>>>> parsing starts at the beginning of each catch clause, and parses
>>>>>>> both paths at branches, backward branches can be ignored. In the
>>>>>>> following example, the target of the branch is 10. The
>>>>>>> implementation first parses the bytecodes between 2 and 10 and
>>>>>>> then, if needed, parses the bytecodes from 10 onward. In this
>>>>>>> case, it would detect the return at 2 and throw a VerifyError
>>>>>>> before parsing the bytecodes from 10 on.
>>>>>>>
>>>>>>> 1 goto 10
>>>>>>> 2 return
>>>>>>> 3 ifeq 2
>>>>>>> ...
>>>>>>> 10
>>>>>>>
>>>>>>> In this example the branch target is 3, the code would parse the
>>>>>>> the bytecodes between 1 and 3. It would see the athrow but would
>>>>>>> also parse the bytecodes from 3 onward to make sure that that path
>>>>>>> also ends in an athrow.
>>>>>>>
>>>>>>> 1 ifeql 3
>>>>>>> 2 athrow
>>>>>>> 3 ...
>>>>>>>
>>>>>>> One advantage of this approach is that it handles the following
>>>>>>> case. The TRY block at 5-6 does an unconditional goto around the
>>>>>>> catch clause to the throw at 10. But, since both paths at the
>>>>>>> goto are parsed, the return at 8 is detected and VerifyError is
>>>>>>> thrown. This enables the implementation to avoid checking for
>>>>>>> exception handlers for every bytecode in a catch handler.
>>>>>>>
>>>>>>> 1 public TryNestedTry2() {
>>>>>>> 2 try {
>>>>>>> 3 super();
>>>>>>> 4 } catch(Exception e) {
>>>>>>> 5 try {
>>>>>>> 6 xxxx();
>>>>>>> 7 } catch(Exception f) {
>>>>>>> 8 return;
>>>>>>> 9 }
>>>>>>> 10 throw e;
>>>>>>> 11 }
>>>>>>> 12 }
>>>>>>>
>>>>>>> The second major change adds support for catch clauses containing
>>>>>>> TRY blocks. When the implementation finds an athrow it checks to
>>>>>>> see if the athrow is contained in TRY blocks. If it is, then it
>>>>>>> pushes the starting bytecodes for those TRY blocks' catch handlers
>>>>>>> onto a separate stack. The bytecodes in those catch handlers are
>>>>>>> then parsed after all normal paths have been parsed. For example,
>>>>>>> it would return VerifyError for this case:
>>>>>>>
>>>>>>> static int n = 1;
>>>>>>> public TryNestedTryOK() {
>>>>>>> try {
>>>>>>> super();
>>>>>>> } catch(java.lang.VerifyError g) {
>>>>>>> try {
>>>>>>> throw g;
>>>>>>> } catch(Exception h) {
>>>>>>> n++;
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>>
>>>>>>> On 7/30/2014 4:53 PM, harold seigel wrote:
>>>>>>>
>>>>>>> Hi Dean,
>>>>>>>
>>>>>>> Thanks for finding this problem. I will look into issues with
>>>>>>> backward branches. Perhaps, if I scan every forward path,
>>>>>>> backward branches can be ignored.
>>>>>>>
>>>>>>> Harold
>>>>>>>
>>>>>>> On 7/29/2014 6:28 PM, Dean Long wrote:
>>>>>>>
>>>>>>> Checking the algorithm, it looks like there is a change of
>>>>>>> an infinite loop:
>>>>>>>
>>>>>>> L0:
>>>>>>> goto L2 // 2: set_interval(L2, code_length);
>>>>>>> start_bc_offset:
>>>>>>> L2:
>>>>>>> ifle L0 // 1: set_interval(L0, L2)
>>>>>>> L3:
>>>>>>> code_length:
>>>>>>>
>>>>>>> dl
>>>>>>>
>>>>>>> On 7/29/2014 9:47 AM, harold seigel wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this updated webrev for bug 8050485.
>>>>>>> This update does not use recursion. Instead, it uses
>>>>>>> a GrowableArray to push and pop bytecode intervals
>>>>>>> when parsing if*, goto*, tableswitch, lookupswitch and
>>>>>>> athrow bytecodes. This updated webrev was tested using
>>>>>>> the same tests as the previous weberv.
>>>>>>>
>>>>>>> The updated webrev is at:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8050485_2/
>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8050485_2/>
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>> On 7/24/2014 1:55 PM, harold seigel wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this verifier fix for bug 8050485.
>>>>>>>
>>>>>>> The fix for JDK-8035119
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8035119>
>>>>>>> broke existing tools, such as NetBeans Profiler,
>>>>>>> that generate bytecodes which place TRY blocks
>>>>>>> around constructor calls to super() and this().
>>>>>>> The purpose of that fix was to prevent exception
>>>>>>> handlers from handling exceptions thrown from
>>>>>>> super() and this(), and returning malformed
>>>>>>> objects to the callers of the constructors.
>>>>>>>
>>>>>>> The NB Profiler prevents malformed objects from
>>>>>>> being returned to the constructors' callers by
>>>>>>> having the exception handlers re-throw the exceptions.
>>>>>>>
>>>>>>> The purpose of this fix is to allow a TRY block
>>>>>>> around a constructor's call to super() and this(),
>>>>>>> provided that all code paths in the TRY block's
>>>>>>> exception handlers terminate with a throw. This
>>>>>>> prevents malformed objects from being returned and
>>>>>>> does not break tools like NB Profiler.
>>>>>>>
>>>>>>> The fix works by parsing the bytecodes inside of
>>>>>>> the exception handlers, making sure that all code
>>>>>>> paths end in an 'athrow' bytecode. Otherwise, it
>>>>>>> throws a VerifyError exception. This parsing is
>>>>>>> only done when the verifier detects a
>>>>>>> constructor's call to super() or this() from
>>>>>>> inside of a TRY block.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8050485
>>>>>>> Open webrev:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8050485/
>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8050485/>
>>>>>>>
>>>>>>> The fix was tested with the JCK lang, vm, and
>>>>>>> api/java_lang tests, the UTE verifier and quick
>>>>>>> tests, the JTREG hotspot tests, and additional
>>>>>>> tests with constructor calls to super() and this()
>>>>>>> from inside of a TRY block, including one provided
>>>>>>> by NB Profiler.
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> twitter-icon-large.png
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Keith McGuigan
>>>>>>>
>>>>>>> @kamggg
>>>>>>>
>>>>>>> kmcguigan at twitter.com <mailto:kmcguigan at twitter.com>
>>>>>>>
More information about the hotspot-runtime-dev
mailing list