RFR 8050485: super() in a try block in a ctor causes VerifyError
Keith McGuigan
kmcguigan at twitter.com
Fri Aug 8 17:15:13 UTC 2014
Hi Harold,
What would you think of creating a:
struct bci_interval {
u4 start;
u4 end;
};
... and then using a growableArray<bci_interval> instead. Saves you from
having to do %2 asserts and guarantees you won't ever accidentally pop off
just one value.
On Fri, Aug 8, 2014 at 9:03 AM, harold seigel <harold.seigel at oracle.com>
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