RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Tue Aug 12 12:47:28 UTC 2014
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>
>>>>>
>
More information about the hotspot-runtime-dev
mailing list