Please review. The following fails with
current openjdk, passes with proposed fix.
/*
* Copyright 2008 Sun Microsystems, Inc. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but
WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file
that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License
version
* 2 along with this work; if not, write to the Free Software
Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa
Clara,
* CA 95054 USA or visit www.sun.com if you need additional
information or
* have any questions.
*/
/*
* @test
* @bug 6761678
* @summary Check properties of Annotations returned from
* getParameterAnnotations, including freedom from security
* exceptions.
* @author Martin Buchholz
*/
import java.lang.annotation.Annotation;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Method;
import java.security.Permission;
import java.security.Policy;
import java.security.ProtectionDomain;
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.FIELD, ElementType.PARAMETER })
@interface Named {
String value();
}
public class ParameterAnnotations {
// A security policy that differs from the default only in that it
// allows a security manager to be uninstalled.
static class MyPolicy extends Policy {
final Policy defaultPolicy;
MyPolicy(Policy defaultPolicy) {
this.defaultPolicy = defaultPolicy;
}
public boolean implies(ProtectionDomain pd, Permission p) {
return p.getName().equals("setSecurityManager") ||
defaultPolicy.implies(pd, p);
}
}
public void nop(@Named("foo") Object foo,
@Named("bar") Object bar) {
}
void test(String[] args) throws Throwable {
// Test without a security manager
test1();
// Test with a security manager
Policy defaultPolicy = Policy.getPolicy();
Policy.setPolicy(new MyPolicy(defaultPolicy));
System.setSecurityManager(new SecurityManager());
try {
test1();
} finally {
System.setSecurityManager(null);
Policy.setPolicy(defaultPolicy);
}
}
void test1() throws Throwable {
for (Method m : thisClass.getMethods()) {
if (m.getName().equals("nop")) {
Annotation[][] ann = m.getParameterAnnotations();
equal(ann.length, 2);
Annotation foo = ann[0][0];
Annotation bar = ann[1][0];
equal(foo.toString(), "@Named(value=foo)");
equal(bar.toString(), "@Named(value=bar)");
check(foo.equals(foo));
check(! foo.equals(bar));
}
}
}
//--------------------- Infrastructure ---------------------------
volatile int passed = 0, failed = 0;
void pass() {passed++;}
void fail() {failed++; Thread.dumpStack();}
void fail(String msg) {System.err.println(msg); fail();}
void unexpected(Throwable t) {failed++; t.printStackTrace();}
void check(boolean cond) {if (cond) pass(); else fail();}
void equal(Object x, Object y) {
if (x == null ? y == null : x.equals(y)) pass();
else fail(x + " not equal to " + y);}
static Class<?> thisClass = new
Object(){}.getClass().getEnclosingClass();
public static void main(String[] args) throws Throwable {
try {thisClass.getMethod("instanceMain",String[].class)
.invoke(thisClass.newInstance(), (Object) args);}
catch (Throwable e) {throw e.getCause();}}
public void instanceMain(String[] args) throws Throwable {
try {test(args);} catch (Throwable t) {unexpected(t);}
System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
if (failed > 0) throw new AssertionError("Some tests failed");}
}
Martin
On Mon, Oct 20, 2008 at 19:01, Joe Darcy <[EMAIL PROTECTED]> wrote:
PS Martin, I haven't written tests that setup and use a security
manager,
etc., that should go along with this fix. Having a stand-alone
regression
test along those lines would be helpful.
Thanks,
-Joe
On 10/20/08 06:59 PM, Joe Darcy wrote:
Hello.
On 10/16/08 12:46 PM, Martin Buchholz wrote:
Hi all,
This is a bug report with fix.
Joe Darcy, please file a bug and review this change,
I've filed 6761678 "(ann) SecurityException in
AnnotationInvocationHandler.getMemberMethods" for this issue. The
problem
seems similar to 6370080 "(ann) Method.getAnnotations() sometimes
throw
SecurityException: doPrivileged or javadoc missing?," which was
fixed in JDK
6 and a JDK 5 update release.
Martin, can you, Toby, and Josh review any other uses of reflection
in the
src/share/classes/sun/reflect/annotation package for similar
problems so we
can address any other such issues now?
I've looked over the change and the use of getMemberMethods in
equalsImpl
and don't see a problem. However, I'd like the security team to
give it a
once over too; security-dev folk, please take a look at this.
Thanks,
-Joe
and perhaps provide a small test case (it is impractical
to share the test we have at Google).
Description:
sun/reflect/annotation/AnnotationInvocationHandler.java.getMemberMethods
might throw if there is a security manager that does not allow
getDeclaredMethods.
The author of this code (Josh Bloch) confirms that the intent was
for the
doPrivileged block in this method to prevent security exceptions.
The methods cannot escape to untrusted code.
Evaluation:
Yes. Fix provided courtesy of Toby Reyelts and Josh Bloch at Google.
# HG changeset patch
# User martin
# Date 1224185752 25200
# Node ID 68730f05449cd4f39ce1cb82adc6c4e57f87554f
# Parent 214ebdcf7252d4862449fe0ae295e6c60a127315
SecurityException in AnnotationInvocationHandler.getMemberMethods
Summary: Move call to getDeclaredMethods inside doPrivileged
Reviewed-by:
Contributed-by: [EMAIL PROTECTED]
diff --git
a/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
b/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
---
a/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
+++
b/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
@@ -272,14 +272,14 @@
*/
private Method[] getMemberMethods() {
if (memberMethods == null) {
- final Method[] mm = type.getDeclaredMethods();
- AccessController.doPrivileged(new
PrivilegedAction<Void>() {
- public Void run() {
- AccessibleObject.setAccessible(mm, true);
- return null;
- }
- });
- memberMethods = mm;
+ memberMethods = AccessController.doPrivileged(
+ new PrivilegedAction<Method[]>() {
+ public Method[] run() {
+ final Method[] mm =
type.getDeclaredMethods();
+ AccessibleObject.setAccessible(mm, true);
+ return mm;
+ }
+ });
}
return memberMethods;
}
