[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: Request for review: 8015385 Remove RelaxAccessControlCheck for JDK 8 bytecodes
From: Karen Kinnear <karen.kinnear () oracle ! com>
Date: 2013-05-31 20:06:38
Message-ID: FA36CCA6-358B-4D04-83DF-992824369A90 () oracle ! com
[Download RAW message or body]
Harold, David,
Code looks good. I would go with the way this is written.
I could argue you either direction in terms of which matters most - accessor or \
accessee version number accessor - the older bytecode accessor will be surprised \
with the change in behavior
- the original point of the flag was to avoid surprise if the accessee changed \
without the accessor knowing
- if the accessor rebuilds they have a chance to fix the problem, so that would \
argue for the accessor version check
accessee - the newer code wants to enforce access checking - that is why we have \
the issue to start with
- so they would expect their new change to actually make a difference
I think following the model the flag was already using will be the least confusing to \
customers. We will need to document this.
I think we gave the flag to give a breathing period for folks to make the appropriate \
code changes. Classfile 49 to 52 seems like long enough to make the transition. There \
is still a workaround -Xverify:none, so there is a backup plan in the field.
I also read the code as having the logic we want.
thanks,
Karen
On May 31, 2013, at 9:14 AM, harold seigel wrote:
> Hi David,
>
> Thanks for the review. Please see inline comments.
>
> Thanks, Harold
>
> On 5/31/2013 3:31 AM, David Holmes wrote:
> > Hi Harold,
> >
> > I'm not sure what the actual rules for this should be in terms of the accessor \
> > and accessee having different versions. But isn't the main issue the version of \
> > the accessee?
> I'm also not sure what the actual rules should be. I chose the stricter path of \
> requiring the access check if either class is >= 52 in part because the existing \
> checks for pre 49 class files require both accessor and accessee to be < 49. If \
> people feel strongly I can change it.
> >
> > Further your change will now require RelaxAccessControlCheck be true for pre 49 \
> > class files, but I thought this old code was supposed to continue working?
> Here's the code change using numbers instead of enums to make it easier to see the \
> parentheses. I think it shows that RelaxAccessControlCheck is not needed for pre \
> 49 class files.
> if ((RelaxAccessControlCheck && accessor_ik->major_version() < 52 && \
> accessee_ik->major_version() < 52) || (accessor_ik->major_version() < 49 && \
> accessee_ik->major_version() < 49)) { return classloader_only &&
> Verifier::relax_verify_for(accessor_ik->class_loader()) &&
> accessor_ik->protection_domain() == accessee_ik->protection_domain() &&
> accessor_ik->class_loader() == accessee_ik->class_loader();
> } else {
> return false;
> }
> }
>
> >
> > Thanks,
> > David
> >
> > On 29/05/2013 11:25 PM, harold seigel wrote:
> > > Hi,
> > >
> > > Please review this fix for bug 8015385.
> > >
> > > This fix removes the use of the RelaxAccessControlCheck option for
> > > bytecode versions >= 52 and so requires the stricter access checking
> > > that previously could be avoided by using this option. Note that the
> > > original purpose of this option was for use with pre-Java_1_5 bytecodes
> > > that did not contain the necessary accessors needed to properly access
> > > private fields.
> > >
> > > This change was tested with JCK lang and vm tests, UTE vm.quick.testlist
> > > and vm.mlvm.testlist tests, JPRT, and jtreg tests. Tests were run on
> > > Linux and Solaris.
> > >
> > > Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8015385/
> > > <http://cr.openjdk.java.net/%7Ehseigel/bug_8015385/>
> > >
> > > Bug link at http://bugs.sun.com/view_bug.do?bug_id=8015385
> > >
> > > Thanks, Harold
>
[Attachment #3 (unknown)]
<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
-webkit-line-break: after-white-space; ">Harold, David,<div><br></div><div>Code looks \
good. I would go with the way this is written.<br><div><br></div><div>I could argue \
you either direction in terms of which matters most - accessor or accessee version \
number</div><div> accessor - the older bytecode accessor will be surprised with \
the change in behavior</div><div> - the original point of the flag was \
to avoid surprise if the accessee changed without the accessor \
knowing</div><div> - if the accessor rebuilds they have a chance to fix \
the problem, so that would argue for the accessor version \
check</div><div><br></div><div> accessee - the newer code wants to enforce \
access checking - that is why we have the issue to start with </div><div> \
- so they would expect their new change to actually make a \
difference</div><div><br></div><div>I think following the model the flag was already \
using will be the least confusing to customers. We will need</div><div>to document \
this.</div><div><br></div><div>I think we gave the flag to give a breathing period \
for folks to make the appropriate code changes. Classfile 49</div><div>to 52 seems \
like long enough to make the transition. There is still a workaround -Xverify:none, \
so there is a</div><div>backup plan in the field.</div><div><br></div><div>I also \
read the code as having the logic we \
want.</div><div><br></div><div>thanks,</div><div>Karen</div><div><br></div><div><div><div>On \
May 31, 2013, at 9:14 AM, harold seigel wrote:</div><br \
class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
Hi David,<br>
<br>
Thanks for the review. Please see inline comments.<br>
<br>
Thanks, Harold<br>
<br>
<div class="moz-cite-prefix">On 5/31/2013 3:31 AM, David Holmes
wrote:<br>
</div>
<blockquote cite="mid:51A851CA.2080207@oracle.com" type="cite">Hi
Harold,
<br>
<br>
I'm not sure what the actual rules for this should be in terms of
the accessor and accessee having different versions. But isn't the
main issue the version of the accessee?
<br>
</blockquote>
I'm also not sure what the actual rules should be. I chose the
stricter path of requiring the access check if either class is >=
52 in part because the existing checks for pre 49 class files
require both accessor and accessee to be < 49. If people feel
strongly I can change it. <br>
<blockquote cite="mid:51A851CA.2080207@oracle.com" type="cite">
<br>
Further your change will now require RelaxAccessControlCheck be
true for pre 49 class files, but I thought this old code was
supposed to continue working?
<br>
</blockquote>
Here's the code change using numbers instead of enums to make it
easier to see the parentheses. I think it shows that
RelaxAccessControlCheck is not needed for pre 49 class files.<br>
<br>
if (<b><font color="#ff0000">(</font></b>RelaxAccessControlCheck
&& accessor_ik->major_version() < 52 &&
accessee_ik->major_version() < 52<b><font color="#ff0000">)</font></b>
<b>||</b><br>
<b> </b><b><font \
color="#009900">(</font></b>accessor_ik->major_version() < 49 && \
accessee_ik->major_version() < 49<b><font color="#009900">)</font></b><font \
color="#330033">)</font> {<br> return classloader_only \
&&<br> \
Verifier::relax_verify_for(accessor_ik->class_loader()) &&<br>
accessor_ik->protection_domain() ==
accessee_ik->protection_domain() &&<br>
accessor_ik->class_loader() ==
accessee_ik->class_loader();<br>
} else {<br>
return false;<br>
}<br>
}<br>
<br>
<blockquote cite="mid:51A851CA.2080207@oracle.com" type="cite">
<br>
Thanks,
<br>
David
<br>
<br>
On 29/05/2013 11:25 PM, harold seigel wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Please review this fix for bug 8015385.
<br>
<br>
This fix removes the use of the RelaxAccessControlCheck option
for
<br>
bytecode versions >= 52 and so requires the stricter access
checking
<br>
that previously could be avoided by using this option. Note that
the
<br>
original purpose of this option was for use with pre-Java_1_5
bytecodes
<br>
that did not contain the necessary accessors needed to properly
access
<br>
private fields.
<br>
<br>
This change was tested with JCK lang and vm tests, UTE
vm.quick.testlist
<br>
and vm.mlvm.testlist tests, JPRT, and jtreg tests. Tests were
run on
<br>
Linux and Solaris.
<br>
<br>
Open webrev at <a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~hseigel/bug_8015385/">http://cr.openjdk.java.net/~hseigel/bug_8015385/</a>
<br>
<a class="moz-txt-link-rfc2396E" \
href="http://cr.openjdk.java.net/%7Ehseigel/bug_8015385/"><http://cr.openjdk.java.net/%7Ehseigel/bug_8015385/></a>
<br>
<br>
Bug link at <a class="moz-txt-link-freetext" \
href="http://bugs.sun.com/view_bug.do?bug_id=8015385">http://bugs.sun.com/view_bug.do?bug_id=8015385</a>
<br>
<br>
Thanks, Harold
<br>
</blockquote>
</blockquote>
<br>
</div>
</blockquote></div><br></div></div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic