RFR(s): 8034999 change rmidRunning to a simple lookup (RMI test library)

[Stuart Marks]

Hi all,

Please review this change to remove a redundant timing-retry loop from the 
rmidRunning() routine of the RMI test library. It is replaced with a simple rmid 
registry lookup that returns status immediately, without retries.


Bug:

https://bugs.openjdk.java.net/browse/JDK-8034999

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8034999/webrev.0/

Thanks,

s'marks

Re: RFR: Even more gcc warnings (8035287)

[Mikael Vidstedt]

Thanks for the review Phil!

Cheers,
Mikael

On 2014-02-18 17:37, Phil Race wrote:

> Anything else I should do/test?

No. Sounds fine.

-phil.

On 2/18/14 5:30 PM, Mikael Vidstedt wrote:


On 2014-02-15 09:31, Phil Race wrote:

Looks OK to me although I just realised there's no bug ID here


Bug created:

https://bugs.openjdk.java.net/browse/JDK-8035287

FWIW I develop on WIndows, Mac & Linux and I've noticed widely 
divergent

things that the compilers on these platforms warn about. Warning
free on Linux might not mean warning free on Windows.
Also, assuming you develop on Linux might want to check if any of 
these make

the VS compiler less happy about anything.


Acknowledged - not all platforms/compilers complain about the same 
thing(s). I tried my best to manually verify that no new warnings are 
introduced by building on the usual suspect platforms and grep 
through the warnings.





* src/solaris/native/sun/awt/awt_Font.c

Comparisons with string literals is undefined behavior - keep track 
of whether the string should be freed explicitly with a boolean 
instead.

Gosh .. that code must be from 1996 or thereabouts.


I hope touching it doesn't mean I own it ;)


Anything else I should do/test?

Cheers,
Mikael



-phil.

On 2/15/14 8:37 AM, Mikael Vidstedt wrote:

Corrected link - this webrev is based on jdk9/client:

http://cr.openjdk.java.net/~mikael/webrevs/jdk-warnings/webrev.01/webrev/ 



Cheers,
Mikael

On Feb 14, 2014, at 17:54, Mikael Vidstedt 
 wrote:



All,

A drive-by set of warning fixes:

http://cr.openjdk.java.net/~mikael/webrevs/jdk-warnings/jdk-warnings/webrev.00/ 



Highlights:

* src/share/native/com/sun/java/util/jar/pack/bands.cpp

Set the size of the array explicitly to increase likelihood of 
enum and struct array being in sync. Arguably this should be 
changed to use the (new) [] =  instead.


Initialize all the fields in the (redundant) terminator struct 
explicitly.


Remove unused macro.

* src/share/native/sun/java2d/opengl/OGLContext.c

Get the prototype for jio_snprintf from jvm.h to address an 
implicit declaration.


* src/solaris/native/sun/awt/awt_Font.c

Comparisons with string literals is undefined behavior - keep 
track of whether the string should be freed explicitly with a 
boolean instead.


* src/solaris/native/sun/awt/awt_LoadLibrary.c

The macro is supposed to expand to a void function declaration, 
but forgets to actually add the "void".


Cheers,
Mikael

Re: RFR: Even more gcc warnings (8035287)

[Phil Race]

> Anything else I should do/test?

No. Sounds fine.

-phil.

On 2/18/14 5:30 PM, Mikael Vidstedt wrote:


On 2014-02-15 09:31, Phil Race wrote:

Looks OK to me although I just realised there's no bug ID here


Bug created:

https://bugs.openjdk.java.net/browse/JDK-8035287


FWIW I develop on WIndows, Mac & Linux and I've noticed widely divergent
things that the compilers on these platforms warn about. Warning
free on Linux might not mean warning free on Windows.
Also, assuming you develop on Linux might want to check if any of 
these make

the VS compiler less happy about anything.


Acknowledged - not all platforms/compilers complain about the same 
thing(s). I tried my best to manually verify that no new warnings are 
introduced by building on the usual suspect platforms and grep through 
the warnings.





* src/solaris/native/sun/awt/awt_Font.c

Comparisons with string literals is undefined behavior - keep track 
of whether the string should be freed explicitly with a boolean 
instead.

Gosh .. that code must be from 1996 or thereabouts.


I hope touching it doesn't mean I own it ;)


Anything else I should do/test?

Cheers,
Mikael



-phil.

On 2/15/14 8:37 AM, Mikael Vidstedt wrote:

Corrected link - this webrev is based on jdk9/client:

http://cr.openjdk.java.net/~mikael/webrevs/jdk-warnings/webrev.01/webrev/ 



Cheers,
Mikael

On Feb 14, 2014, at 17:54, Mikael Vidstedt 
 wrote:



All,

A drive-by set of warning fixes:

http://cr.openjdk.java.net/~mikael/webrevs/jdk-warnings/jdk-warnings/webrev.00/ 



Highlights:

* src/share/native/com/sun/java/util/jar/pack/bands.cpp

Set the size of the array explicitly to increase likelihood of enum 
and struct array being in sync. Arguably this should be changed to 
use the (new) [] =  instead.


Initialize all the fields in the (redundant) terminator struct 
explicitly.


Remove unused macro.

* src/share/native/sun/java2d/opengl/OGLContext.c

Get the prototype for jio_snprintf from jvm.h to address an 
implicit declaration.


* src/solaris/native/sun/awt/awt_Font.c

Comparisons with string literals is undefined behavior - keep track 
of whether the string should be freed explicitly with a boolean 
instead.


* src/solaris/native/sun/awt/awt_LoadLibrary.c

The macro is supposed to expand to a void function declaration, but 
forgets to actually add the "void".


Cheers,
Mikael

Re: RFR: Even more gcc warnings (8035287)

[Mikael Vidstedt]

On 2014-02-15 09:31, Phil Race wrote:

Looks OK to me although I just realised there's no bug ID here


Bug created:

https://bugs.openjdk.java.net/browse/JDK-8035287


FWIW I develop on WIndows, Mac & Linux and I've noticed widely divergent
things that the compilers on these platforms warn about. Warning
free on Linux might not mean warning free on Windows.
Also, assuming you develop on Linux might want to check if any of 
these make

the VS compiler less happy about anything.


Acknowledged - not all platforms/compilers complain about the same 
thing(s). I tried my best to manually verify that no new warnings are 
introduced by building on the usual suspect platforms and grep through 
the warnings.





* src/solaris/native/sun/awt/awt_Font.c

Comparisons with string literals is undefined behavior - keep track 
of whether the string should be freed explicitly with a boolean instead.

Gosh .. that code must be from 1996 or thereabouts.


I hope touching it doesn't mean I own it ;)


Anything else I should do/test?

Cheers,
Mikael



-phil.

On 2/15/14 8:37 AM, Mikael Vidstedt wrote:

Corrected link - this webrev is based on jdk9/client:

http://cr.openjdk.java.net/~mikael/webrevs/jdk-warnings/webrev.01/webrev/ 



Cheers,
Mikael

On Feb 14, 2014, at 17:54, Mikael Vidstedt 
 wrote:



All,

A drive-by set of warning fixes:

http://cr.openjdk.java.net/~mikael/webrevs/jdk-warnings/jdk-warnings/webrev.00/ 



Highlights:

* src/share/native/com/sun/java/util/jar/pack/bands.cpp

Set the size of the array explicitly to increase likelihood of enum 
and struct array being in sync. Arguably this should be changed to 
use the (new) [] =  instead.


Initialize all the fields in the (redundant) terminator struct 
explicitly.


Remove unused macro.

* src/share/native/sun/java2d/opengl/OGLContext.c

Get the prototype for jio_snprintf from jvm.h to address an implicit 
declaration.


* src/solaris/native/sun/awt/awt_Font.c

Comparisons with string literals is undefined behavior - keep track 
of whether the string should be freed explicitly with a boolean 
instead.


* src/solaris/native/sun/awt/awt_LoadLibrary.c

The macro is supposed to expand to a void function declaration, but 
forgets to actually add the "void".


Cheers,
Mikael

Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl

[huizhe wang]

On 2/18/2014 10:05 AM, Daniel Fuchs wrote:

Thanks Joe,

+1

There's a small alignment issue in SerializationTest.java line 152.


That's a debug I added a moment ago, removed.



Also the

   if (condition) {
   }
   else {
   }

formatting in the source files is bizarre, but if it can help with
future merges I have no issues.


That's from Xerces (they use Eclipse). I've seen them do this in pure 
format changesets, that is, they'd change "} else {" to the above.


NetBeans format would have been:
   if (condition) {
   } else {
   }

I personally like The NetBeans format. But it will take a while before 
we could get jaxp code formatted properly since in many cases the 
relevant files are very big (over 3000 lines in this case). Hitting 
NetBeans format can generate a lot of noises to the webrev.




No need to regenerate a webrev.


Thanks,
Joe



-- daniel

On 2/18/14 6:42 PM, huizhe wang wrote:


On 2/18/2014 3:39 AM, Daniel Fuchs wrote:

On 2/17/14 11:17 PM, huizhe wang wrote:

Thanks Daniel.

I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.

http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/


Hi Joe,

The changes look good.

In the serialization test I wonder if it would be better
to simply replace:

  53 {
  54 filePath =
SerializationTest.class.getResource("SerializationTest.java").getPath(); 


  55 int pos = filePath.lastIndexOf('/');
  56 filePath = filePath.substring(0, pos + 1);
  57 }

with:

{
 filePath = System.getProperty("test.src", ".");
}


Ok, I changed that to:

   54 filePath = System.getProperty("test.src");
   55 if (filePath == null) {
   56 //current directory
   57 filePath = System.getProperty("user.dir");
   58 }
   59 filePath += File.separator;


Thanks,
Joe



best regards,

-- daniel



Thanks,
Joe

On 2/17/2014 8:23 AM, Daniel Fuchs wrote:

Hi Joe,

Sorry for the late reply...

On 2/14/14 6:35 PM, huizhe wang wrote:

Hi All,

I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration.
I've
created such files for jdk6,7,8 and 9, and manually run the test, 
that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. 
In the

JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past
JDK10, we
could consider add serialization files for JDK10.

The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I 
added

transient since it's the right thing to do.

Adding fields is a compatible change in accordance with Java Object
Serialization Spec
. 






Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:

   InputStream streamIn = 
SerializationTest.class.getResourceAsStream(

   javaVersion + FILENAME_CAL);
   ObjectInputStream objectinputstream = new
ObjectInputStream(streamIn);

The reason is that URL.getPath() is an URL path - not a file path,
so it
can contain encoded characters (such as %20) that FileInputStream 
will

not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.

Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values,  false for boolean etc...)
[I am not 100% sure but it will not hurt to check]

best regards,

-- daniel




Thanks,
Joe

On 2/13/2014 6:23 AM, Alan Bateman wrote:

On 13/02/2014 08:18, huizhe wang wrote:

Hi Alan, Lance, and Daniel,

The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility.
But
in reality it itself is causing significant incompatibility as 
Alan

pointed out below and we discussed previously. I've removed the
revision from the patch as a result.

Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Thanks for dropping the serialization change as it was just not 
going

to work the way you had intended.

I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl 

Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl

[Daniel Fuchs]

Thanks Joe,

+1

There's a small alignment issue in SerializationTest.java line 152.

Also the

   if (condition) {
   }
   else {
   }

formatting in the source files is bizarre, but if it can help with
future merges I have no issues.

No need to regenerate a webrev.

-- daniel

On 2/18/14 6:42 PM, huizhe wang wrote:


On 2/18/2014 3:39 AM, Daniel Fuchs wrote:

On 2/17/14 11:17 PM, huizhe wang wrote:

Thanks Daniel.

I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.

http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/


Hi Joe,

The changes look good.

In the serialization test I wonder if it would be better
to simply replace:

  53 {
  54 filePath =
SerializationTest.class.getResource("SerializationTest.java").getPath();
  55 int pos = filePath.lastIndexOf('/');
  56 filePath = filePath.substring(0, pos + 1);
  57 }

with:

{
 filePath = System.getProperty("test.src", ".");
}


Ok, I changed that to:

   54 filePath = System.getProperty("test.src");
   55 if (filePath == null) {
   56 //current directory
   57 filePath = System.getProperty("user.dir");
   58 }
   59 filePath += File.separator;


Thanks,
Joe



best regards,

-- daniel



Thanks,
Joe

On 2/17/2014 8:23 AM, Daniel Fuchs wrote:

Hi Joe,

Sorry for the late reply...

On 2/14/14 6:35 PM, huizhe wang wrote:

Hi All,

I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration.
I've
created such files for jdk6,7,8 and 9, and manually run the test, that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past
JDK10, we
could consider add serialization files for JDK10.

The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I added
transient since it's the right thing to do.

Adding fields is a compatible change in accordance with Java Object
Serialization Spec
.




Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:

   InputStream streamIn = SerializationTest.class.getResourceAsStream(
   javaVersion + FILENAME_CAL);
   ObjectInputStream objectinputstream = new
ObjectInputStream(streamIn);

The reason is that URL.getPath() is an URL path - not a file path,
so it
can contain encoded characters (such as %20) that FileInputStream will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.

Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values,  false for boolean etc...)
[I am not 100% sure but it will not hurt to check]

best regards,

-- daniel




Thanks,
Joe

On 2/13/2014 6:23 AM, Alan Bateman wrote:

On 13/02/2014 08:18, huizhe wang wrote:

Hi Alan, Lance, and Daniel,

The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility.
But
in reality it itself is causing significant incompatibility as Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.

Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/

Thanks for dropping the serialization change as it was just not going
to work the way you had intended.

I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not clear why they aren't transient.

I have not studied the rest of the changes but I think Daniel and
Lance are.

-Alan

Re: RFR(S): 8034087: XML parser may overwrite element content if that content falls onto the border of an entity scanner buffer

[huizhe wang]

Looks good to me.

Thanks,
Joe

On 2/18/2014 1:43 AM, Volker Simonis wrote:

Hi,

I've moved the test to test/javax/xml/jaxp/parsers/8027359 as requested:

http://cr.openjdk.java.net/~simonis/webrevs/8034087_1/

Can I please get a review for this change.

Thank you and best regards,
Volker


On Wed, Feb 12, 2014 at 7:25 PM, huizhe wang  wrote:

Ok, understand.

Thanks,
Joe


On 2/11/2014 11:40 PM, Schreiber, Steffen wrote:

Hi Joe,

Yes, that's certainly true, if the test is only to show the issue.
We intended the test to work as a regression test as well and wanted to
check the effects of the buffer boundaries on more possible places, i.e.
before/at/after special characters like <, > or / as well as inside tags and
content.

We can easily adapt the loop, if runtime is a concern here.

Regards,
Steffen

-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Sent: Dienstag, 11. Februar 2014 22:57
To: Volker Simonis
Cc: Alan Bateman; Schreiber, Steffen; Java Core Libs
Subject: Re: RFR(S): 8034087: XML parser may overwrite element content if
that content falls onto the border of an entity scanner buffer

Hi Volker,

I agree with the approach below and jdk9/dev is the better forest.

For the test itself, I would suggest reducing the following loop to 1 or
2 cases:

   for (int i = 0; i < testString.length(); i++) {
   test(createDocument(testString.toString(), i), ""+ i);
   }


when i=7, the problem starts to show. It's sufficient to demonstrate the
issue then by just entering 7. It's unnecessary to run the test 43 times.


Thanks,
Joe

On 2/11/2014 9:00 AM, Volker Simonis wrote:

Hi Alan,

you're right. I initially didn't saw the test because I just looked at
the change in the jaxp repository.

If it will be approved, I'll put the test in the same directory like
the other test (i.e. test/javax/xml/jaxp/parsers/8027359).

And yes, my plan was to get approval for both, the tests and the fix,
when asking for the permission to downport to jdk8u-dev and jdk7u-dev.

Thanks,
Volker


On Tue, Feb 11, 2014 at 5:44 PM, Alan Bateman 
wrote:

On 11/02/2014 14:57, Volker Simonis wrote:

Hi,

after opening this bug yesterday for an issue found by my colleague
Steffen Schreiber we realized that this is actually a duplicate of
"8027359: XML parser returns incorrect parsing results"
(https://bugs.openjdk.java.net/browse/JDK-8027359).

While there's no need now to submit a patch anymore,  we'd
nevertheless like to contribute at least our test case for this issue:

http://cr.openjdk.java.net/~simonis/webrevs/8034087/

The webrev is against jdk9-client but we'd like to also downport this
test to jdk7 and jdk8 to track that the fix for 8027359 will be
correctly downported to these releases as well.

I will sponsor this change if somebody would be so kind to review it.


I'll leave it to Joe Wang to comment on the test but just to mention
that
jdk9/dev is probably a better forest to aim for because that is where
the
XML (and its tests) usually go.  Also I wonder if it might be better to
put
it in the same directory as the test that Joe pushed with the change?

If you are getting approval to push to jdk8u-dev and jdk7u-dev then it
might
be better to request a backport of Joe's change at the same time.

-Alan.

Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl

[huizhe wang]

On 2/18/2014 3:39 AM, Daniel Fuchs wrote:

On 2/17/14 11:17 PM, huizhe wang wrote:

Thanks Daniel.

I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.

http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/


Hi Joe,

The changes look good.

In the serialization test I wonder if it would be better
to simply replace:

  53 {
  54 filePath = 
SerializationTest.class.getResource("SerializationTest.java").getPath();

  55 int pos = filePath.lastIndexOf('/');
  56 filePath = filePath.substring(0, pos + 1);
  57 }

with:

{
 filePath = System.getProperty("test.src", ".");
}


Ok, I changed that to:

  54 filePath = System.getProperty("test.src");
  55 if (filePath == null) {
  56 //current directory
  57 filePath = System.getProperty("user.dir");
  58 }
  59 filePath += File.separator;


Thanks,
Joe



best regards,

-- daniel



Thanks,
Joe

On 2/17/2014 8:23 AM, Daniel Fuchs wrote:

Hi Joe,

Sorry for the late reply...

On 2/14/14 6:35 PM, huizhe wang wrote:

Hi All,

I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration. 
I've

created such files for jdk6,7,8 and 9, and manually run the test, that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past 
JDK10, we

could consider add serialization files for JDK10.

The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I added
transient since it's the right thing to do.

Adding fields is a compatible change in accordance with Java Object
Serialization Spec
. 





Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:

   InputStream streamIn = SerializationTest.class.getResourceAsStream(
   javaVersion + FILENAME_CAL);
   ObjectInputStream objectinputstream = new 
ObjectInputStream(streamIn);


The reason is that URL.getPath() is an URL path - not a file path, 
so it

can contain encoded characters (such as %20) that FileInputStream will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.

Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values,  false for boolean etc...)
[I am not 100% sure but it will not hurt to check]

best regards,

-- daniel




Thanks,
Joe

On 2/13/2014 6:23 AM, Alan Bateman wrote:

On 13/02/2014 08:18, huizhe wang wrote:

Hi Alan, Lance, and Daniel,

The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility. 
But

in reality it itself is causing significant incompatibility as Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.

Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/

Thanks for dropping the serialization change as it was just not going
to work the way you had intended.

I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not clear why they aren't transient.

I have not studied the rest of the changes but I think Daniel and
Lance are.

-Alan

Re: Minor com.sun.jndi.dns cleanup

[Florian Weimer]

On 02/17/2014 10:17 PM, Alan Bateman wrote:

On 17/02/2014 20:01, Florian Weimer wrote:

In the DnsName case, the same exception is used for parsing
user-supplied strings and data from the wire, and strictly speaking,
InvalidNameException should be used only in the former case.

Yes, I think the exceptions in DnsName should be re-examined too
(doesn't have to be this patch of course). For RecordRecord then it
would be good to see if there is a better NamingException, the closest
seems to be CommunicationException with an appropriate cause but maybe
there are more suitable choices.


Thanks for the bug number and these suggestions.  The new version is 
here: 


I had to adjust the checked exceptions, so I wrapped the 
InvalidNameException from DnsName in a CommunicationException as well.


I eliminated the recursion and from the name parser and added yet 
another check for invalid label types. (Other implementations treat 
extended label types as errors as well, so there is no need to implement 
them here.)


Is this code actually used?  (I discovered it because I looked at 
DatagramSocket users in the JDK.)  If it is, there are some other fixes 
besides the ArrayIndexOutOfBoundsException handling that would make sense.


--
Florian Weimer / Red Hat Product Security Team

Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl

[Daniel Fuchs]

On 2/17/14 11:17 PM, huizhe wang wrote:

Thanks Daniel.

I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.

http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/


Hi Joe,

The changes look good.

In the serialization test I wonder if it would be better
to simply replace:

  53 {
  54 filePath = 
SerializationTest.class.getResource("SerializationTest.java").getPath();

  55 int pos = filePath.lastIndexOf('/');
  56 filePath = filePath.substring(0, pos + 1);
  57 }

with:

{
 filePath = System.getProperty("test.src", ".");
}

best regards,

-- daniel



Thanks,
Joe

On 2/17/2014 8:23 AM, Daniel Fuchs wrote:

Hi Joe,

Sorry for the late reply...

On 2/14/14 6:35 PM, huizhe wang wrote:

Hi All,

I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration. I've
created such files for jdk6,7,8 and 9, and manually run the test, that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past JDK10, we
could consider add serialization files for JDK10.

The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I added
transient since it's the right thing to do.

Adding fields is a compatible change in accordance with Java Object
Serialization Spec
.



Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:

   InputStream streamIn = SerializationTest.class.getResourceAsStream(
   javaVersion + FILENAME_CAL);
   ObjectInputStream objectinputstream = new ObjectInputStream(streamIn);

The reason is that URL.getPath() is an URL path - not a file path, so it
can contain encoded characters (such as %20) that FileInputStream will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.

Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values,  false for boolean etc...)
[I am not 100% sure but it will not hurt to check]

best regards,

-- daniel




Thanks,
Joe

On 2/13/2014 6:23 AM, Alan Bateman wrote:

On 13/02/2014 08:18, huizhe wang wrote:

Hi Alan, Lance, and Daniel,

The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility. But
in reality it itself is causing significant incompatibility as Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.

Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/

Thanks for dropping the serialization change as it was just not going
to work the way you had intended.

I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not clear why they aren't transient.

I have not studied the rest of the changes but I think Daniel and
Lance are.

-Alan

Re: RFR(S): 8034087: XML parser may overwrite element content if that content falls onto the border of an entity scanner buffer

[Volker Simonis]

Hi,

I've moved the test to test/javax/xml/jaxp/parsers/8027359 as requested:

http://cr.openjdk.java.net/~simonis/webrevs/8034087_1/

Can I please get a review for this change.

Thank you and best regards,
Volker


On Wed, Feb 12, 2014 at 7:25 PM, huizhe wang  wrote:
> Ok, understand.
>
> Thanks,
> Joe
>
>
> On 2/11/2014 11:40 PM, Schreiber, Steffen wrote:
>>
>> Hi Joe,
>>
>> Yes, that's certainly true, if the test is only to show the issue.
>> We intended the test to work as a regression test as well and wanted to
>> check the effects of the buffer boundaries on more possible places, i.e.
>> before/at/after special characters like <, > or / as well as inside tags and
>> content.
>>
>> We can easily adapt the loop, if runtime is a concern here.
>>
>> Regards,
>> Steffen
>>
>> -Original Message-
>> From: huizhe wang [mailto:huizhe.w...@oracle.com]
>> Sent: Dienstag, 11. Februar 2014 22:57
>> To: Volker Simonis
>> Cc: Alan Bateman; Schreiber, Steffen; Java Core Libs
>> Subject: Re: RFR(S): 8034087: XML parser may overwrite element content if
>> that content falls onto the border of an entity scanner buffer
>>
>> Hi Volker,
>>
>> I agree with the approach below and jdk9/dev is the better forest.
>>
>> For the test itself, I would suggest reducing the following loop to 1 or
>> 2 cases:
>>
>>   for (int i = 0; i < testString.length(); i++) {
>>   test(createDocument(testString.toString(), i), ""+ i);
>>   }
>>
>>
>> when i=7, the problem starts to show. It's sufficient to demonstrate the
>> issue then by just entering 7. It's unnecessary to run the test 43 times.
>>
>>
>> Thanks,
>> Joe
>>
>> On 2/11/2014 9:00 AM, Volker Simonis wrote:
>>>
>>> Hi Alan,
>>>
>>> you're right. I initially didn't saw the test because I just looked at
>>> the change in the jaxp repository.
>>>
>>> If it will be approved, I'll put the test in the same directory like
>>> the other test (i.e. test/javax/xml/jaxp/parsers/8027359).
>>>
>>> And yes, my plan was to get approval for both, the tests and the fix,
>>> when asking for the permission to downport to jdk8u-dev and jdk7u-dev.
>>>
>>> Thanks,
>>> Volker
>>>
>>>
>>> On Tue, Feb 11, 2014 at 5:44 PM, Alan Bateman 
>>> wrote:

 On 11/02/2014 14:57, Volker Simonis wrote:
>
> Hi,
>
> after opening this bug yesterday for an issue found by my colleague
> Steffen Schreiber we realized that this is actually a duplicate of
> "8027359: XML parser returns incorrect parsing results"
> (https://bugs.openjdk.java.net/browse/JDK-8027359).
>
> While there's no need now to submit a patch anymore,  we'd
> nevertheless like to contribute at least our test case for this issue:
>
> http://cr.openjdk.java.net/~simonis/webrevs/8034087/
>
> The webrev is against jdk9-client but we'd like to also downport this
> test to jdk7 and jdk8 to track that the fix for 8027359 will be
> correctly downported to these releases as well.
>
> I will sponsor this change if somebody would be so kind to review it.
>
 I'll leave it to Joe Wang to comment on the test but just to mention
 that
 jdk9/dev is probably a better forest to aim for because that is where
 the
 XML (and its tests) usually go.  Also I wonder if it might be better to
 put
 it in the same directory as the test that Joe pushed with the change?

 If you are getting approval to push to jdk8u-dev and jdk7u-dev then it
 might
 be better to request a backport of Joe's change at the same time.

 -Alan.
>
>