RFR 8050485: super() in a try block in a ctor causes VerifyError
Keith McGuigan
kmcguigan at twitter.com
Tue Aug 12 13:48:32 UTC 2014
I would consider adding a ResourceMark in ends_in_athrow(), and an initial
size of 50 for those GrowableArrays seems a little overkill, but otherwise
it LGTM.
On Tue, 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>
>>>>>>
>>>>>>
>>
>
--
[image: twitter-icon-large.png]
Keith McGuigan
@kamggg
kmcguigan at twitter.com
More information about the hotspot-runtime-dev
mailing list