[Patch][JDK10] Use Class.getPackageName() where possible
Hey, I noticed two places in the codebase that could call JDK 9's new method Class.getPackageName(). Would be happy if this is sponsored in case the patch is correct. Cheers, Christoph ====== PATCH ======= diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Fri Nov 03 01:47:04 2017 +0100 @@ -1587,11 +1587,7 @@ * Returns package name of given class. */ private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + return cl.getPackageName(); } /** diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Fri Nov 03 01:47:04 2017 +0100 @@ -1034,11 +1034,8 @@ // do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName(); if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg));
On 11/2/17 5:55 PM, Christoph Dreis wrote:
Hey,
I noticed two places in the codebase that could call JDK 9's new method Class.getPackageName().
It's good to see JDK code be updated to use Class::getPackageName. One thing to pay attention is that Class.getPackageName() returns "java.lang" for primitive type and void. Your patch fixing ObjectStreamClass::getPackageName and Proxy::checkNewProxyPermission look fine. There are other places that can be converted. Do you mind updating java.io.ObjectInputFilter::matchesPackage and ClassLoader::checkPackageAccess? I may miss there are other places in java.base. I can sponsor this once you have an updated patch. Mandy
Would be happy if this is sponsored in case the patch is correct.
Cheers, Christoph
====== PATCH ======= diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Fri Nov 03 01:47:04 2017 +0100 @@ -1587,11 +1587,7 @@ * Returns package name of given class. */ private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + return cl.getPackageName(); }
/** diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Fri Nov 03 01:47:04 2017 +0100 @@ -1034,11 +1034,8 @@
// do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName();
if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg));
Hi Mandy,
It's good to see JDK code be updated to use Class::getPackageName. One thing to pay attention is that Class.getPackageName() returns "java.lang" for primitive type and void. Your patch fixing ObjectStreamClass::getPackageName and Proxy::checkNewProxyPermission look fine. There are other places that can be converted. Do you mind updating java.io.ObjectInputFilter::matchesPackage and ClassLoader::checkPackageAccess? I may miss there are other places in java.base.
Thanks - I updated both as you suggested to use Class::getPackageName. Please find the updated patch below. There is also a public static VerifyAccess::getPackageName which seems to be not working with arrays at all and as far as I can see is at least not used inside the JDK itself. Should this be deprecated maybe in favor of Class.getPackageName() or also adjusted to use it (which would mean different return values than before as you already noted)? Cheers, Christoph ====== PATCH ======= diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Fri Nov 03 08:38:15 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); } /** diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Fri Nov 03 08:38:15 2017 +0100 @@ -1587,11 +1587,7 @@ * Returns package name of given class. */ private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + return cl.getPackageName(); } /** diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/ClassLoader.java --- a/src/java.base/share/classes/java/lang/ClassLoader.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/ClassLoader.java Fri Nov 03 08:38:15 2017 +0100 @@ -672,12 +672,11 @@ return; } - final String name = cls.getName(); - final int i = name.lastIndexOf('.'); - if (i != -1) { + final String packageName = cls.getPackageName(); + if (!packageName.isEmpty()) { AccessController.doPrivileged(new PrivilegedAction<>() { public Void run() { - sm.checkPackageAccess(name.substring(0, i)); + sm.checkPackageAccess(packageName); return null; } }, new AccessControlContext(new ProtectionDomain[] {pd})); diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Fri Nov 03 08:38:15 2017 +0100 @@ -1034,11 +1034,8 @@ // do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName(); if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg));
On 11/3/17 1:06 AM, Christoph Dreis wrote:
Thanks - I updated both as you suggested to use Class::getPackageName. Please find the updated patch below.
I have created https://bugs.openjdk.java.net/browse/JDK-8190733 for this patch.
There is also a public static VerifyAccess::getPackageName which seems to be not working with arrays at all and as far as I can see is at least not used inside the JDK itself. Should this be deprecated maybe in favor of Class.getPackageName() or also adjusted to use it (which would mean different return values than before as you already noted)?
VerifyAccess::getPackageName is unused in jdk8u-dev neither. So I think it can be removed. No deprecation is needed since this is JDK internal API and I hardly think anyone is depending on it. Mandy
Cheers, Christoph
====== PATCH ======= diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Fri Nov 03 08:38:15 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); }
/**
diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Fri Nov 03 08:38:15 2017 +0100 @@ -1587,11 +1587,7 @@ * Returns package name of given class. */ private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + return cl.getPackageName(); }
/**
diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/ClassLoader.java --- a/src/java.base/share/classes/java/lang/ClassLoader.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/ClassLoader.java Fri Nov 03 08:38:15 2017 +0100 @@ -672,12 +672,11 @@ return; }
- final String name = cls.getName(); - final int i = name.lastIndexOf('.'); - if (i != -1) { + final String packageName = cls.getPackageName(); + if (!packageName.isEmpty()) { AccessController.doPrivileged(new PrivilegedAction<>() { public Void run() { - sm.checkPackageAccess(name.substring(0, i)); + sm.checkPackageAccess(packageName); return null; } }, new AccessControlContext(new ProtectionDomain[] {pd}));
diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Fri Nov 03 08:38:15 2017 +0100 @@ -1034,11 +1034,8 @@
// do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName();
if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg));
The private static helper in ObjectStreamClass became a oneliner and can be removed and the callsites can transform from getPackageName(c) to c.getPackageName(). Gruss Bernd -- http://bernd.eckenfels.net _____________________________ From: mandy chung <mandy.chung@oracle.com<mailto:mandy.chung@oracle.com>> Sent: Samstag, November 4, 2017 1:12 AM Subject: Re: [Patch][JDK10] Use Class.getPackageName() where possible To: Christoph Dreis <christoph.dreis@freenet.de<mailto:christoph.dreis@freenet.de>>, 'Core-Libs-Dev' <core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>> On 11/3/17 1:06 AM, Christoph Dreis wrote:
Thanks - I updated both as you suggested to use Class::getPackageName. Please find the updated patch below.
I have created https://bugs.openjdk.java.net/browse/JDK-8190733 for this patch.
There is also a public static VerifyAccess::getPackageName which seems to be not working with arrays at all and as far as I can see is at least not used inside the JDK itself. Should this be deprecated maybe in favor of Class.getPackageName() or also adjusted to use it (which would mean different return values than before as you already noted)?
VerifyAccess::getPackageName is unused in jdk8u-dev neither. So I think it can be removed. No deprecation is needed since this is JDK internal API and I hardly think anyone is depending on it. Mandy
Cheers, Christoph
====== PATCH ======= diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Fri Nov 03 08:38:15 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); }
/**
diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Fri Nov 03 08:38:15 2017 +0100 @@ -1587,11 +1587,7 @@ * Returns package name of given class. */ private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + return cl.getPackageName(); }
/**
diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/ClassLoader.java --- a/src/java.base/share/classes/java/lang/ClassLoader.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/ClassLoader.java Fri Nov 03 08:38:15 2017 +0100 @@ -672,12 +672,11 @@ return; }
- final String name = cls.getName(); - final int i = name.lastIndexOf('.'); - if (i != -1) { + final String packageName = cls.getPackageName(); + if (!packageName.isEmpty()) { AccessController.doPrivileged(new PrivilegedAction<>() { public Void run() { - sm.checkPackageAccess(name.substring(0, i)); + sm.checkPackageAccess(packageName); return null; } }, new AccessControlContext(new ProtectionDomain[] {pd}));
diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Fri Nov 03 08:38:15 2017 +0100 @@ -1034,11 +1034,8 @@
// do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName();
if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg));
On 11/3/17 6:52 PM, Bernd Eckenfels wrote:
The private static helper in ObjectStreamClass became a oneliner and can be removed and the callsites can transform from getPackageName(c) to c.getPackageName().
Good catch. we should clean that up. Christoph - can you send a updated patch to remove ObjectStreamClass::getPackageName and replace the calls with Class::getPackageName and also remove VerifyAccess::getPackageName? thanks Mandy
Gruss Bernd -- http://bernd.eckenfels.net _____________________________ From: mandy chung <mandy.chung@oracle.com<mailto:mandy.chung@oracle.com>> Sent: Samstag, November 4, 2017 1:12 AM Subject: Re: [Patch][JDK10] Use Class.getPackageName() where possible To: Christoph Dreis <christoph.dreis@freenet.de<mailto:christoph.dreis@freenet.de>>, 'Core-Libs-Dev' <core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>>
On 11/3/17 1:06 AM, Christoph Dreis wrote:
Thanks - I updated both as you suggested to use Class::getPackageName. Please find the updated patch below. I have created https://bugs.openjdk.java.net/browse/JDK-8190733 for this patch.
There is also a public static VerifyAccess::getPackageName which seems to be not working with arrays at all and as far as I can see is at least not used inside the JDK itself. Should this be deprecated maybe in favor of Class.getPackageName() or also adjusted to use it (which would mean different return values than before as you already noted)? VerifyAccess::getPackageName is unused in jdk8u-dev neither. So I think it can be removed. No deprecation is needed since this is JDK internal API and I hardly think anyone is depending on it.
Mandy
Cheers, Christoph
====== PATCH ======= diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Fri Nov 03 08:38:15 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); }
/**
diff -r 438e0c9f2f17 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Fri Nov 03 08:38:15 2017 +0100 @@ -1587,11 +1587,7 @@ * Returns package name of given class. */ private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + return cl.getPackageName(); }
/**
diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/ClassLoader.java --- a/src/java.base/share/classes/java/lang/ClassLoader.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/ClassLoader.java Fri Nov 03 08:38:15 2017 +0100 @@ -672,12 +672,11 @@ return; }
- final String name = cls.getName(); - final int i = name.lastIndexOf('.'); - if (i != -1) { + final String packageName = cls.getPackageName(); + if (!packageName.isEmpty()) { AccessController.doPrivileged(new PrivilegedAction<>() { public Void run() { - sm.checkPackageAccess(name.substring(0, i)); + sm.checkPackageAccess(packageName); return null; } }, new AccessControlContext(new ProtectionDomain[] {pd}));
diff -r 438e0c9f2f17 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Oct 30 17:49:33 2017 -0700 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Fri Nov 03 08:38:15 2017 +0100 @@ -1034,11 +1034,8 @@
// do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName();
if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg));
Hi,
On 11/3/17 6:52 PM, Bernd Eckenfels wrote:
The private static helper in ObjectStreamClass became a oneliner and can be removed and the callsites can transform from getPackageName(c) to c.getPackageName(). Good catch. we should clean that up. Christoph - can you send a updated patch to remove ObjectStreamClass::getPackageName and replace the calls with Class::getPackageName and also remove VerifyAccess::getPackageName?
Please find the updated patch below. Cheers, Christoph ======= PATCH ======= diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Tue Nov 07 15:44:36 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); } /** diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectStreamClass.java --- a/src/java.base/share/classes/java/io/ObjectStreamClass.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java Tue Nov 07 15:44:36 2017 +0100 @@ -1580,18 +1580,7 @@ */ private static boolean packageEquals(Class<?> cl1, Class<?> cl2) { return (cl1.getClassLoader() == cl2.getClassLoader() && - getPackageName(cl1).equals(getPackageName(cl2))); - } - - /** - * Returns package name of given class. - */ - private static String getPackageName(Class<?> cl) { - String s = cl.getName(); - int i = s.lastIndexOf('['); - i = (i < 0) ? 0 : i + 2; - int j = s.lastIndexOf('.'); - return (i < j) ? s.substring(i, j) : ""; + cl1.getPackageName().equals(cl2.getPackageName())); } /** diff -r 67aa34b019e1 src/java.base/share/classes/java/lang/ClassLoader.java --- a/src/java.base/share/classes/java/lang/ClassLoader.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/lang/ClassLoader.java Tue Nov 07 15:44:36 2017 +0100 @@ -675,12 +675,11 @@ return; } - final String name = cls.getName(); - final int i = name.lastIndexOf('.'); - if (i != -1) { + final String packageName = cls.getPackageName(); + if (!packageName.isEmpty()) { AccessController.doPrivileged(new PrivilegedAction<>() { public Void run() { - sm.checkPackageAccess(name.substring(0, i)); + sm.checkPackageAccess(packageName); return null; } }, new AccessControlContext(new ProtectionDomain[] {pd})); diff -r 67aa34b019e1 src/java.base/share/classes/java/lang/reflect/Proxy.java --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java Tue Nov 07 15:44:36 2017 +0100 @@ -1034,11 +1034,8 @@ // do permission check if the caller is in a different runtime package // of the proxy class - int n = proxyClass.getName().lastIndexOf('.'); - String pkg = (n == -1) ? "" : proxyClass.getName().substring(0, n); - - n = caller.getName().lastIndexOf('.'); - String callerPkg = (n == -1) ? "" : caller.getName().substring(0, n); + String pkg = proxyClass.getPackageName(); + String callerPkg = caller.getPackageName(); if (pcl != ccl || !pkg.equals(callerPkg)) { sm.checkPermission(new ReflectPermission("newProxyInPackage." + pkg)); diff -r 67aa34b019e1 src/java.base/share/classes/sun/invoke/util/VerifyAccess.java --- a/src/java.base/share/classes/sun/invoke/util/VerifyAccess.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/sun/invoke/util/VerifyAccess.java Tue Nov 07 15:44:36 2017 +0100 @@ -332,16 +332,6 @@ return Objects.equals(class1.getPackageName(), class2.getPackageName()); } - /** Return the package name for this class. - */ - public static String getPackageName(Class<?> cls) { - assert (!cls.isArray()); - String name = cls.getName(); - int dot = name.lastIndexOf('.'); - if (dot < 0) return ""; - return name.substring(0, dot); - } - /** * Test if two classes are defined as part of the same package member (top-level class). * If this is true, they can share private access with each other.
On 11/7/17 6:48 AM, Christoph Dreis wrote:
======= PATCH ======= diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Tue Nov 07 15:44:36 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); }
This is correct but we could simplify this. We can modify the callers to drop a trailing "." from the pkg parameter. I took the liberty to revise it a little. http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8190733/webrev.00/ Roger - can you take a look at the change in ObjectInputFilter? Mandy
Hi Mandy, yes, the revision in the webrev is correct; the trailing '.' in the pkg was just to ensure it was a package prefix. +1, Roger On 11/7/2017 6:00 PM, mandy chung wrote:
On 11/7/17 6:48 AM, Christoph Dreis wrote:
======= PATCH ======= diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Tue Nov 07 15:44:36 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); }
This is correct but we could simplify this. We can modify the callers to drop a trailing "." from the pkg parameter. I took the liberty to revise it a little.
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8190733/webrev.00/
Roger - can you take a look at the change in ObjectInputFilter?
Mandy
Thanks Roger. Christoph - your patch has been pushed to jdk/jdk repo. Mandy On 11/8/17 10:11 AM, Roger Riggs wrote:
Hi Mandy,
yes, the revision in the webrev is correct; the trailing '.' in the pkg was just to ensure it was a package prefix.
+1, Roger
On 11/7/2017 6:00 PM, mandy chung wrote:
On 11/7/17 6:48 AM, Christoph Dreis wrote:
======= PATCH ======= diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectInputFilter.java --- a/src/java.base/share/classes/java/io/ObjectInputFilter.java Mon Nov 06 17:48:00 2017 -0800 +++ b/src/java.base/share/classes/java/io/ObjectInputFilter.java Tue Nov 07 15:44:36 2017 +0100 @@ -656,8 +656,8 @@ * otherwise {@code false} */ private static boolean matchesPackage(Class<?> c, String pkg) { - String n = c.getName(); - return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() - 1; + String n = c.getPackageName(); + return n.length() == pkg.length() - 1 && n.startsWith(pkg); }
This is correct but we could simplify this. We can modify the callers to drop a trailing "." from the pkg parameter. I took the liberty to revise it a little.
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8190733/webrev.00/
Roger - can you take a look at the change in ObjectInputFilter?
Mandy
Hi Mandy,
Christoph - your patch has been pushed to jdk/jdk repo.
I appreciate the sponsoring. Thanks! Cheers, Christoph
participants (4)
-
Bernd Eckenfels
-
Christoph Dreis
-
mandy chung
-
Roger Riggs