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

[Keith McGuigan]

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>
wrote:

> Hi,
>
> Please review this latest webrev for fixing 8050485.  The webrev is at:
> http://cr.openjdk.java.net/~hseigel/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/
>>>>
>>>> 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/
>>>>>
>>>>> 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
>>>>>
>>>>
>>>>
>>>
>>
>


-- 

[image: twitter-icon-large.png]

Keith McGuigan

@kamggg

kmcguigan at twitter.com