[Patch][JDK10] Use Class.getPackageName() where possible

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch][JDK10] Use Class.getPackageName() where possible

Christoph Dreis
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));

Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

Mandy Chung


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));
>

Reply | Threaded
Open this post in threaded view
|

RE: [Patch][JDK10] Use Class.getPackageName() where possible

Christoph Dreis
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));


Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

Mandy Chung


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));
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

Bernd Eckenfels-4
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 <[hidden email]<mailto:[hidden email]>>
Sent: Samstag, November 4, 2017 1:12 AM
Subject: Re: [Patch][JDK10] Use Class.getPackageName() where possible
To: Christoph Dreis <[hidden email]<mailto:[hidden email]>>, 'Core-Libs-Dev' <[hidden email]<mailto:[hidden email]>>




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));
>
>



Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

Mandy Chung

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 <[hidden email]<mailto:[hidden email]>>
> Sent: Samstag, November 4, 2017 1:12 AM
> Subject: Re: [Patch][JDK10] Use Class.getPackageName() where possible
> To: Christoph Dreis <[hidden email]<mailto:[hidden email]>>, 'Core-Libs-Dev' <[hidden email]<mailto:[hidden email]>>
>
>
>
>
> 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));
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

RE: [Patch][JDK10] Use Class.getPackageName() where possible

Christoph Dreis
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.


Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

Mandy Chung


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
Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

roger riggs
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

Reply | Threaded
Open this post in threaded view
|

Re: [Patch][JDK10] Use Class.getPackageName() where possible

Mandy Chung
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
>

Reply | Threaded
Open this post in threaded view
|

RE: [Patch][JDK10] Use Class.getPackageName() where possible

Christoph Dreis
Hi Mandy,

> Christoph - your patch has been pushed to jdk/jdk repo.

I appreciate the sponsoring. Thanks!

Cheers,
Christoph