RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Mon Aug 11 18:47:44 UTC 2014
Hi Keith,
Thanks for the review. I hope to use Dean's suggestion of just pushing
and popping the starting offset. If I end up needing to push and pop an
interval then I think your suggestion of pushing and popping a struct
makes sense.
Thanks, Harold
On 8/8/2014 1:15 PM, Keith McGuigan wrote:
> 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 <mailto: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/
> <http://cr.openjdk.java.net/%7Ehseigel/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>
> <mailto: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/>
> <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/>
> <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/>
> <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>
> <mailto:kmcguigan at twitter.com <mailto:kmcguigan at twitter.com>>
>
>
>
>
>
>
> --
>
> 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