RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Tue Aug 12 17:55:24 UTC 2014
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