RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Mon Aug 11 18:18:06 UTC 2014
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?
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