RFR 8050485: super() in a try block in a ctor causes VerifyError
Dean Long
dean.long at oracle.com
Tue Aug 12 20:03:27 UTC 2014
Sure, sounds good.
dl
On 8/12/2014 10:55 AM, harold seigel wrote:
> Hi Dean,
>
> I decided to leave out the additional checks because I think the
> benefit is small and the only known users of this do not have branches
> in their catch clauses. I hope this is satisfactory.
>
> Thanks, Harold
>
> On 8/11/2014 9:36 PM, Dean Long wrote:
>> On 8/11/2014 11:18 AM, harold seigel wrote:
>>> Hi Dean,
>>>
>>> Thanks for the review.
>>>
>>> I think that you are correct that I need only push the start bci and
>>> not an interval. Then, when popping the stack, just scan from the
>>> popped value to the end of the method. I don't think I need to add
>>> any additional checks. The additional checks might prevent scanning
>>> the same bytecodes twice, especially in the case of a forward
>>> branch, but would not affect validity.
>>>
>>> Does that make sense?
>>>
>> Sure, but I couldn't think of an example where additional checks
>> would negatively affect validity .
>>
>> dl
>>
>>> Thanks, Harold
>>>
>>> On 8/8/2014 6:51 PM, Dean Long wrote:
>>>> Harold, I think you should be able to push just the start bci
>>>> instead of an interval, and then scan until you see either 1) a
>>>> previously parsed
>>>> bci, or 2) a bci that is already pushed on the stack. What do you
>>>> think?
>>>>
>>>> dl
>>>>
>>>> On 8/8/2014 6: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