RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Fri Aug 8 13:03:36 UTC 2014
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