RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Tue Aug 12 20:08:39 UTC 2014
Hi Dean,
Thank you for all your reviews and all your help.
Harold
On 8/12/2014 4:03 PM, Dean Long wrote:
> 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