RFR 8050485: super() in a try block in a ctor causes VerifyError

Dean Long dean.long at oracle.com
Fri Aug 8 22:51:11 UTC 2014


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