Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

On Thu, Jul 14, 2016 at 4:04 PM, Mandy Chung  wrote:
>
>> On Jul 12, 2016, at 9:54 PM, Volker Simonis  wrote:
>>
>> Please find the new webrev at:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v2/
>>
>
> Looks good.
>
> Nit: maybe better to rename the parameter to “version”.
>   79 static List parseVersionNumbers(String versionNumber) {
>
> No need to generate a new webrev.
>

Thanks for the review! I'll do so.

Regards,
Volker


> thanks
> Mandy
>

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

Thanks Iris!

On Tue, Jul 12, 2016 at 9:01 PM, Iris Clark  wrote:
> Hi.
>
>>> Please find the new webrev at:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v2/
>>
>> +1
>>
>>> Any other comments?
>
>> Only to note that this adds a validation check that we don't have
>> trailing zeros, which I was recently made aware of is being
>> reconsidered, see https://bugs.openjdk.java.net/browse/JDK-8156907
>> - if so that implementation will likely have to revert parts of
>> this validation.
>
> I've added a comment to this effect in 8156907. That changeset will
> need to include updates this code and associated regression test.
>
> Regards,
> iris

RE: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Iris Clark]

Hi.

>> Please find the new webrev at:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v2/
> 
> +1
> 
>> Any other comments?

> Only to note that this adds a validation check that we don't have
> trailing zeros, which I was recently made aware of is being
> reconsidered, see https://bugs.openjdk.java.net/browse/JDK-8156907
> - if so that implementation will likely have to revert parts of
> this validation.

I've added a comment to this effect in 8156907. That changeset will
need to include updates this code and associated regression test.

Regards,
iris

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Claes Redestad]

On 07/12/2016 03:54 PM, Volker Simonis wrote:

Please find the new webrev at:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v2/


+1


Any other comments?


Only to note that this adds a validation check that we don't have
trailing zeros, which I was recently made aware of is being reconsidered,
see https://bugs.openjdk.java.net/browse/JDK-8156907 - if so that
implementation will likely have to revert parts of this validation.

Thanks!

/Claes



Regards,
Volker

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Claes Redestad]

On 2016-07-11 18:18, Volker Simonis wrote:

Hi,

here comes a new version of this change. I've tried to address most of
the concerns/suggestions:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v1/



Looks good. As I'm currently obsessing about startup performance, I did
wish we could rely on (and fix, as a separate issue) the validation
done in build scripts, but there's no real harm in doing proper
validation in runtime as long as we avoid using regex here.



- Finally, I didn't understood the comment about 'List.of might be
preferred over Arrays.asList' so I didn't change that.


Oh, I meant that new List.of(...) can be used as a shorthand for
Arrays.asList(...) in the test. No real difference, just one less
import and a bit cleaner code in my opinion, so please ignore it at
this point.

Thanks!

/Claes



OK to push now (before it gets really over-engineered :)

Thank you and best regards,
Volker

On Thu, Jul 7, 2016 at 9:54 PM, Mandy Chung  wrote:

Hi Volker,

Thanks for adding a new test for it.

Nit: can you wrap the long lines.

As for the bad version, one possible change is to add assert 
Runtime.Version.parse(versionNumber) in parseVersionNumbers method and add -esa 
in @run tag.

It’d be good to convert this to testng test where the dataprovider can show the 
input version and expected returned list.

Mandy


On Jul 7, 2016, at 6:59 AM, Volker Simonis  wrote:

Hi,

can I please have a review for the following change which makes
VersionProps.versionNumbers() testable and the corresponding
regression test:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564/
https://bugs.openjdk.java.net/browse/JDK-8160564

During the review for "8160457: VersionProps.versionNumbers() is
broken" it was suggested to refactor VersionProps.versionNumbers() in
order to make it testable by a regression test. This change does
exactly that. It extracts the implementation of
VersionProps.versionNumbers() into a new method
parseVersionNumbers(String versionNumber) which can be tested from the
associated regression test.

There are still two points to notice:

- VersionProps.versionNumbers() deliberately doesn't check for badly
formatted version strings because it is assumed it will only get valid
input (see discussion for "JDK-816: Runtime.version() cause
startup regressions" at [2]). So we can currently only check that
VersionProps.versionNumbers() accepts all kind of valid version
strings.

- I was a little bit surprised that I could reflectively access and
execute java.lang.VersionProps.parseVersionNumbers() where both the
class and the method are package private. Maybe this is related to
Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
Jigsaw expert, I'd be graceful to anybody explaining me why this is
still so easily possible with Jigsaw?

Thank you and best regards,
Volker


[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html
[2] https://bugs.openjdk.java.net/browse/JDK-816
[3] 
http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Mandy Chung]

> On Jul 12, 2016, at 12:18 AM, Volker Simonis  wrote:
> 
> Hi,
> 
> here comes a new version of this change. I've tried to address most of
> the concerns/suggestions:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v1/
> 

This version looks okay in general.  I suggest to combine the check and 
Integer.parseInt in a single method (e.g. “numberAt” or some other better 
method name).

  71 if (index - prevIndex > 1 &&

may read better as "index > prevIndex”

The new comment can simply say “This method is used by regression tests” that 
should do it.  I would not mention the test name in the source since 
refactoring may make it outdated.

In the new test,

  92 if (error) {
  93 throw new Exception(invalidVersions[i] +
  94 " not recognized as invalid by 
VersionProps.parseVersionNumbers()");
  95 }

An alternative to the above check, it can throw an exception immediate after 
line 82 when  parseVersionNumbers.invoke succeeds.
  
Mandy 

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

Hi,

here comes a new version of this change. I've tried to address most of
the concerns/suggestions:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v1/

1. Added a private 'check()' method to the VersionProps class which
ensures that every single part of a version number starts with a digit
greater zero (i.e. no leading zeros). If this condition isn't
fulfilled, an IllegalArgumentException will be thrown.
2. Check that the final list of version numbers returned by
VersionProps.parseVersionNumbers() has no leading or trailing zeros.
If not, throw an IllegalArgumentException.
3. With the changes 1. and 2. in place we can now check for bad input
in the regression test as well. The test now checks that
VersionProps.parseVersionNumbers() accepts and rejects the same
version strings as  Runtime.Version.parse(). So I think there's no
need for runtime assertions or configure checks any more.
4. Put a comment on VersionProps.parseVersionNumbers() to remind
others that the method is reflectively tests.

- The test copyright is OK. We already have a bunch of tests/files
with similar copyright.
- Wrapped the long lines.
- Moved test from test/java/lang/VersionProps.java to
test/java/lang/Runtime/Version/VersionProps.java
- I would prefer not to convert this test to TestNG for simplicity. I
like to be able to execute a test stand-alone in case of failure,
without any test framework. Moreover the test already prints what it's
doing anyway (see below) and in the case of failure it prints the
initial/expected version strings.

[1]
[1, 2]
[1, 2, 3]
[1, 2, 3, 4]
[1, 0, 0, 1]
[1, 1, 1]
[1, 0, 2, 0, 0, 3, 0, 0, 0, 4, 5, 0, 0, 6]
[101]
[1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 9, 8, 7, 6, 5, 4, 3, 2, 1]
OK - caught bad version string 01
OK - caught bad version string 0.1.2
OK - caught bad version string 1.02.3
OK - caught bad version string 1.2.03.4
OK - caught bad version string 1.0.0.1.0
OK - caught bad version string 1.0.1.0.0
OK - caught bad version string 1.00.1
OK - caught bad version string 1.0.1.00
OK - caught bad version string 1.1.

- Finally, I didn't understood the comment about 'List.of might be
preferred over Arrays.asList' so I didn't change that.

OK to push now (before it gets really over-engineered :)

Thank you and best regards,
Volker

On Thu, Jul 7, 2016 at 9:54 PM, Mandy Chung  wrote:
> Hi Volker,
>
> Thanks for adding a new test for it.
>
> Nit: can you wrap the long lines.
>
> As for the bad version, one possible change is to add assert 
> Runtime.Version.parse(versionNumber) in parseVersionNumbers method and add 
> -esa in @run tag.
>
> It’d be good to convert this to testng test where the dataprovider can show 
> the input version and expected returned list.
>
> Mandy
>
>> On Jul 7, 2016, at 6:59 AM, Volker Simonis  wrote:
>>
>> Hi,
>>
>> can I please have a review for the following change which makes
>> VersionProps.versionNumbers() testable and the corresponding
>> regression test:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564/
>> https://bugs.openjdk.java.net/browse/JDK-8160564
>>
>> During the review for "8160457: VersionProps.versionNumbers() is
>> broken" it was suggested to refactor VersionProps.versionNumbers() in
>> order to make it testable by a regression test. This change does
>> exactly that. It extracts the implementation of
>> VersionProps.versionNumbers() into a new method
>> parseVersionNumbers(String versionNumber) which can be tested from the
>> associated regression test.
>>
>> There are still two points to notice:
>>
>> - VersionProps.versionNumbers() deliberately doesn't check for badly
>> formatted version strings because it is assumed it will only get valid
>> input (see discussion for "JDK-816: Runtime.version() cause
>> startup regressions" at [2]). So we can currently only check that
>> VersionProps.versionNumbers() accepts all kind of valid version
>> strings.
>>
>> - I was a little bit surprised that I could reflectively access and
>> execute java.lang.VersionProps.parseVersionNumbers() where both the
>> class and the method are package private. Maybe this is related to
>> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
>> Jigsaw expert, I'd be graceful to anybody explaining me why this is
>> still so easily possible with Jigsaw?
>>
>> Thank you and best regards,
>> Volker
>>
>>
>> [1] 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html
>> [2] https://bugs.openjdk.java.net/browse/JDK-816
>> [3] 
>> http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes
>

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[John Rose]

On Jul 7, 2016, at 11:05 AM, Martin Buchholz  wrote:

> private static final jdk.internal.misc.Unsafe 
> java.util.concurrent.ConcurrentHashMap.U

Unless the security manager is turned on, you can do setAcc to pick up all 
sorts of private fields.

As Alan points out, it would be good to tighten this up, but that will require 
new logic which somehow avoids breaking existing legitimate uses.

(The new logic can't be some hack like "check for Unsafe".  Point-fixes like 
that just complicate the security model, creating dark corners for black hats 
to work in.)

The JVM and language are loose about mentioning inaccessible (or even 
non-existent) classes in field and method descriptors.
In particular, the JVM does not resolve or access-check names which occur as 
types of fields, parameters, or return values.
So accessing a field of type "Unsafe" (or even "NoSuchType") gets a pass, as 
long as the privacy of the field is somehow taken into account (via setAcc, 
here).

> jdk.internal.misc.Unsafe@35851384

Here we get an object of a non-exported type.  That happens all the time; an 
anonymous inner class or lambda expression does this.

Again, the language and JVM are loose about letting objects of internal type 
"escape" via public APIs.  Arguably this is fundamental to OOP.

> class jdk.internal.misc.Unsafe

Here we inspect the class of the mysterious escaped object.  You can do this to 
AICs and lambdas too.

Our legacy reflection system says that Object.getClass returns the actual class 
of the object, without access check.
Again, that's loose, and difficult to tighten compatibly with existing 
applications.
In particular, adding an access check to getClass (which is the obvious move 
here) will perturb the performance model.
Currently, getClass requires one or two memory references.  An access check is 
likely to more than double that, with noticeable effects on dynamic languages.

Nothing here is impossible to fix, but everything is tricky, because of 
backward compatibility.

> Exception in thread "main" java.lang.IllegalAccessError: class GetUnsafe9
> (in unnamed module @0x649d209a) cannot access class
> jdk.internal.misc.Unsafe (in module java.base) because module java.base
> does not export jdk.internal.misc to unnamed module @0x649d209a
> at GetUnsafe9.main(GetUnsafe9.java:16)

Finally we get an error.  The specific error comes from an invisible cast which 
follows a successful call to the Class.cast method:

   //jdk.internal.misc.Unsafe U = unsafeClass.cast(theUnsafe);
   // ==>
   Object $tem = unsafeClass.cast(theUnsafe);  // descriptor is (Object)Object
   jdk.internal.misc.Unsafe U = (jdk.internal.misc.Unsafe) $tem; // javac 
inserts this cast

The "checkcast" instruction in the second statement fails because it must 
resolve the type Unsafe.

Even if there were other ways to get your hands on a well-typed Unsafe 
instance, as soon as you attempted to resolve an "invokevirtual" against it, 
you'd hit the same class resolution error as "checkcast" did.

The reflection API performs similar checks to the source compiler and JVM 
bytecode executor (verifier & link resolver).
However, since it works on "live" class pointers there is no resolution step to 
catch "bad" class references.
So Class.cast and Method.invoke either have to let the "bad" class operate 
(which Class.cast does) or perform an additional ad hoc access check (which 
Method.invoke does).
The additional access check is useful, but setAccessible nullifies it.
Paradoxically, Class.cast (which doesn't have the ad hoc check) is more 
restrictive than Method.invoke, since javac puts in the invisible cast (forcing 
resolution on Class.cast) while Method.invoke can be neutered by setAccessible.

Again, we are back to finding ways to rein in setAccessible.

HTH
— John

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Mandy Chung]

Hi Volker,

Thanks for adding a new test for it.

Nit: can you wrap the long lines. 

As for the bad version, one possible change is to add assert 
Runtime.Version.parse(versionNumber) in parseVersionNumbers method and add -esa 
in @run tag.

It’d be good to convert this to testng test where the dataprovider can show the 
input version and expected returned list.

Mandy

> On Jul 7, 2016, at 6:59 AM, Volker Simonis  wrote:
> 
> Hi,
> 
> can I please have a review for the following change which makes
> VersionProps.versionNumbers() testable and the corresponding
> regression test:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564/
> https://bugs.openjdk.java.net/browse/JDK-8160564
> 
> During the review for "8160457: VersionProps.versionNumbers() is
> broken" it was suggested to refactor VersionProps.versionNumbers() in
> order to make it testable by a regression test. This change does
> exactly that. It extracts the implementation of
> VersionProps.versionNumbers() into a new method
> parseVersionNumbers(String versionNumber) which can be tested from the
> associated regression test.
> 
> There are still two points to notice:
> 
> - VersionProps.versionNumbers() deliberately doesn't check for badly
> formatted version strings because it is assumed it will only get valid
> input (see discussion for "JDK-816: Runtime.version() cause
> startup regressions" at [2]). So we can currently only check that
> VersionProps.versionNumbers() accepts all kind of valid version
> strings.
> 
> - I was a little bit surprised that I could reflectively access and
> execute java.lang.VersionProps.parseVersionNumbers() where both the
> class and the method are package private. Maybe this is related to
> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
> Jigsaw expert, I'd be graceful to anybody explaining me why this is
> still so easily possible with Jigsaw?
> 
> Thank you and best regards,
> Volker
> 
> 
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html
> [2] https://bugs.openjdk.java.net/browse/JDK-816
> [3] 
> http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Alan Bateman]

On 07/07/2016 19:05, Martin Buchholz wrote:


When jdk9 is released, an army of white, black, grey, and red hats will try
to keep their old Unsafe hacks alive and maybe get their hands on a
jdk.internal.misc.Unsafe.
I assume these Unsafe usages are sun.misc.Unsafe so they should continue 
to work. More details in JEP 260 [1].




Here's some code that tries to do that. The call
to setAccessible succeeds! And the code succeeds in getting hold
of jdk.internal.misc.Unsafe.class and of its instance before finally
failing with IllegalAccessError.  So this may actually be safe, in that
user code may not be able to actually invoke methods on their ill-gotten
Unsafe object, but the intent is probably that they shouldn't be able to
get this far:
The behavior you see is expected. Many us would like setAccessible(true) 
to go away but there are many use-cases that would need alternatives first.


-Alan

[1] http://openjdk.java.net/jeps/260

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Martin Buchholz]

When jdk9 is released, an army of white, black, grey, and red hats will try
to keep their old Unsafe hacks alive and maybe get their hands on a
jdk.internal.misc.Unsafe.  Here's some code that tries to do that. The call
to setAccessible succeeds! And the code succeeds in getting hold
of jdk.internal.misc.Unsafe.class and of its instance before finally
failing with IllegalAccessError.  So this may actually be safe, in that
user code may not be able to actually invoke methods on their ill-gotten
Unsafe object, but the intent is probably that they shouldn't be able to
get this far:

/**
 * javac -Xmodule:java.base GetUnsafe9.java && java GetUnsafe9
 */
public class GetUnsafe9 {
public static void main(String[] args) throws Throwable {
Class klazz = java.util.concurrent.ConcurrentHashMap.class;
for (java.lang.reflect.Field field : klazz.getDeclaredFields()) {
if (field.getName().equals("U")) {
field.setAccessible(true);
System.out.println(field);
Object theUnsafe = field.get(null);
System.out.println(theUnsafe);
Class unsafeClass =
(Class) theUnsafe.getClass();
System.out.println(unsafeClass);
jdk.internal.misc.Unsafe U = unsafeClass.cast(theUnsafe);
}
}
}
}

private static final jdk.internal.misc.Unsafe
java.util.concurrent.ConcurrentHashMap.U
jdk.internal.misc.Unsafe@35851384
class jdk.internal.misc.Unsafe
Exception in thread "main" java.lang.IllegalAccessError: class GetUnsafe9
(in unnamed module @0x649d209a) cannot access class
jdk.internal.misc.Unsafe (in module java.base) because module java.base
does not export jdk.internal.misc to unnamed module @0x649d209a
at GetUnsafe9.main(GetUnsafe9.java:16)



On Thu, Jul 7, 2016 at 7:54 AM, Andrew Dinn  wrote:

> On 07/07/16 14:59, Volker Simonis wrote:
> > - I was a little bit surprised that I could reflectively access and
> > execute java.lang.VersionProps.parseVersionNumbers() where both the
> > class and the method are package private. Maybe this is related to
> > Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
> > Jigsaw expert, I'd be graceful to anybody explaining me why this is
> > still so easily possible with Jigsaw?
>
> Reflective access to non-public classes/members of exported packages is
> unchanged with Jigsaw -- it is subject to the security checks in place
> in previous JDKs but not to a module access check.  So, in this case
> java.lang is an exported package which means you can obtain a handle on
> java.lang.VersionProps.parseVersionNumbers(), call setEnabled(true) and
> then invoke it.
>
> It is only when you try to reflectively access non-public
> classes/members of packages that are not exported by their owning module
> that the check comes into play. So, if your (non-module) code obtains a
> reflective Member for jdk.internal.misc.Unsafe.theUnsafe and calls
> setEnabled(true) you will find that the latter call will not succeed in
> rendering it an accessible handle and an access via that handle will
> fail. That is because java.base does not export package
> jdk.internal.misc so the module check under setEnabled detects that the
> caller is in a different module and refuses to make it accessible.
>
> regards,
>
>
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Claes Redestad]

On 2016-07-07 18:08, Volker Simonis wrote:

Not sure how error checking could or should be improved:
>VersionProps.parseVersionNumbers(String) will throw a NFE on most malformed
>data, technically an IllegalArgumentException. Same thing would happen if
>you ran an invalid string through Runtime.Version.parse(String) (having
>NumberFormatException specified to be thrown there is perhaps redundant).
>So, wouldn't pre-816 behavior be more or less the same if someone
>specified an un-parseable version string during setup?
>
>Perhaps the test could verify that both parseVersionNumber(String) and
>Runtime.Version.parse(String) throws exceptions on the same input.
>

The problem is that they actually don't do that.
parseVersionNumber(String) happily accepts leading zeros or negative
numbers (actually everything that's parsed by Integer.parseInt()).
Handling all these cases in parseVersionNumber(String) would either
make the implementation quite complicated or bring us back to using a
regexp.



You're right, those specific corner-cases would slip through the
816 implementation but not Version.parse, but is handled OK by the
build system by virtue of leading zeroes being stripped from and
negative numbers being refused by --with-version-[major|minor|...]

--with-version-string is checking for negative numbers, but doesn't 
strip leading zeroes: ---with-version-string=00032.0024 gives exactly 
that, which I guess is an issue.


I guess a follow-up RFE to plug that hole in the build and perhaps to 
write a regression test verifying that configure doesn't accept badly

formed version strings is in order.

/Claes

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

On Thu, Jul 7, 2016 at 5:33 PM, Claes Redestad
 wrote:
> Hi Volker,
>
> On 2016-07-07 15:59, Volker Simonis wrote:
>>
>> Hi,
>>
>> can I please have a review for the following change which makes
>> VersionProps.versionNumbers() testable and the corresponding
>> regression test:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564/
>
>
> thanks for doing this!
>
> Changes to VersionProps looks good.
>
> Adding tests to the root of jdk/test/java/lang might be frowned upon. While
> VersionProps is a class of it's own, it is also tightly coupled with
> Runtime.java; perhaps the test could find a home under
> java/lang/Runtime/Version?
>
> For readability List.of might be preferred over Arrays.asList
>
>> https://bugs.openjdk.java.net/browse/JDK-8160564
>>
>> During the review for "8160457: VersionProps.versionNumbers() is
>> broken" it was suggested to refactor VersionProps.versionNumbers() in
>> order to make it testable by a regression test. This change does
>> exactly that. It extracts the implementation of
>> VersionProps.versionNumbers() into a new method
>> parseVersionNumbers(String versionNumber) which can be tested from the
>> associated regression test.
>>
>> There are still two points to notice:
>>
>> - VersionProps.versionNumbers() deliberately doesn't check for badly
>> formatted version strings because it is assumed it will only get valid
>> input (see discussion for "JDK-816: Runtime.version() cause
>> startup regressions" at [2]). So we can currently only check that
>> VersionProps.versionNumbers() accepts all kind of valid version
>> strings.
>
>
> Not sure how error checking could or should be improved:
> VersionProps.parseVersionNumbers(String) will throw a NFE on most malformed
> data, technically an IllegalArgumentException. Same thing would happen if
> you ran an invalid string through Runtime.Version.parse(String) (having
> NumberFormatException specified to be thrown there is perhaps redundant).
> So, wouldn't pre-816 behavior be more or less the same if someone
> specified an un-parseable version string during setup?
>
> Perhaps the test could verify that both parseVersionNumber(String) and
> Runtime.Version.parse(String) throws exceptions on the same input.
>

The problem is that they actually don't do that.
parseVersionNumber(String) happily accepts leading zeros or negative
numbers (actually everything that's parsed by Integer.parseInt()).
Handling all these cases in parseVersionNumber(String) would either
make the implementation quite complicated or bring us back to using a
regexp.

> Thanks!
>
> /Claes
>
>
>>
>> - I was a little bit surprised that I could reflectively access and
>> execute java.lang.VersionProps.parseVersionNumbers() where both the
>> class and the method are package private. Maybe this is related to
>> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
>> Jigsaw expert, I'd be graceful to anybody explaining me why this is
>> still so easily possible with Jigsaw?
>>
>> Thank you and best regards,
>> Volker
>>
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html
>> [2] https://bugs.openjdk.java.net/browse/JDK-816
>> [3]
>> http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes
>>
>

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

Hi Andrew,

thanks a lot for the detailed explanation!

Regards,
Volker


On Thu, Jul 7, 2016 at 4:54 PM, Andrew Dinn  wrote:
> On 07/07/16 14:59, Volker Simonis wrote:
>> - I was a little bit surprised that I could reflectively access and
>> execute java.lang.VersionProps.parseVersionNumbers() where both the
>> class and the method are package private. Maybe this is related to
>> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
>> Jigsaw expert, I'd be graceful to anybody explaining me why this is
>> still so easily possible with Jigsaw?
>
> Reflective access to non-public classes/members of exported packages is
> unchanged with Jigsaw -- it is subject to the security checks in place
> in previous JDKs but not to a module access check.  So, in this case
> java.lang is an exported package which means you can obtain a handle on
> java.lang.VersionProps.parseVersionNumbers(), call setEnabled(true) and
> then invoke it.
>
> It is only when you try to reflectively access non-public
> classes/members of packages that are not exported by their owning module
> that the check comes into play. So, if your (non-module) code obtains a
> reflective Member for jdk.internal.misc.Unsafe.theUnsafe and calls
> setEnabled(true) you will find that the latter call will not succeed in
> rendering it an accessible handle and an access via that handle will
> fail. That is because java.base does not export package
> jdk.internal.misc so the module check under setEnabled detects that the
> caller is in a different module and refuses to make it accessible.
>
> regards,
>
>
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Andrew Dinn]

On 07/07/16 14:59, Volker Simonis wrote:
> - I was a little bit surprised that I could reflectively access and
> execute java.lang.VersionProps.parseVersionNumbers() where both the
> class and the method are package private. Maybe this is related to
> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
> Jigsaw expert, I'd be graceful to anybody explaining me why this is
> still so easily possible with Jigsaw?

Reflective access to non-public classes/members of exported packages is
unchanged with Jigsaw -- it is subject to the security checks in place
in previous JDKs but not to a module access check.  So, in this case
java.lang is an exported package which means you can obtain a handle on
java.lang.VersionProps.parseVersionNumbers(), call setEnabled(true) and
then invoke it.

It is only when you try to reflectively access non-public
classes/members of packages that are not exported by their owning module
that the check comes into play. So, if your (non-module) code obtains a
reflective Member for jdk.internal.misc.Unsafe.theUnsafe and calls
setEnabled(true) you will find that the latter call will not succeed in
rendering it an accessible handle and an access via that handle will
fail. That is because java.base does not export package
jdk.internal.misc so the module check under setEnabled detects that the
caller is in a different module and refuses to make it accessible.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

On Thu, Jul 7, 2016 at 4:26 PM, Alan Bateman  wrote:
> On 07/07/2016 14:59, Volker Simonis wrote:
>
>> :
>>
>> - I was a little bit surprised that I could reflectively access and
>> execute java.lang.VersionProps.parseVersionNumbers() where both the
>> class and the method are package private. Maybe this is related to
>> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
>> Jigsaw expert, I'd be graceful to anybody explaining me why this is
>> still so easily possible with Jigsaw?
>>
>>
> java.lang is an exported package and I assume you are using
> setAccessible(true) too.
>

Yes, I do use setAccessible(true) but I naively expected it to fail.

So does it mean the module system does not prevent me from
reflectively using package private classes/methods from exported
packages?

Thanks,
Volker

> -Alan

Re: RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Alan Bateman]

On 07/07/2016 14:59, Volker Simonis wrote:


:

- I was a little bit surprised that I could reflectively access and
execute java.lang.VersionProps.parseVersionNumbers() where both the
class and the method are package private. Maybe this is related to
Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
Jigsaw expert, I'd be graceful to anybody explaining me why this is
still so easily possible with Jigsaw?


java.lang is an exported package and I assume you are using 
setAccessible(true) too.


-Alan