RFR 8050485: super() in a try block in a ctor causes VerifyError
harold seigel
harold.seigel at oracle.com
Tue Aug 12 14:53:32 UTC 2014
Hi Keith,
Thanks for the review! I'll add the ResourceMark and reduce the initial
sizes of the GrowableArrays to 30 before checking in the fix.
Harold
On 8/12/2014 9:48 AM, Keith McGuigan wrote:
> I would consider adding a ResourceMark in ends_in_athrow(), and an
> initial size of 50 for those GrowableArrays seems a little overkill,
> but otherwise it LGTM.
>
>
>
> On Tue, Aug 12, 2014 at 8:47 AM, harold seigel
> <harold.seigel at oracle.com <mailto:harold.seigel at oracle.com>> wrote:
>
> Hi,
>
> Please review yet another version of the fix for 8050485:
> http://cr.openjdk.java.net/~hseigel/bug_8050485_5/
> <http://cr.openjdk.java.net/%7Ehseigel/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/
> <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