RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

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

RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Aleks Efimov
Hello,

Please, help to review the fix for jaxp bug that fixes SAXException to
correctly set exception cause with 'setCause' method:
http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
I've tried to keep the fix miminal with respect to serial form of this
API class, i.e. kept private 'exception' field.

The new test case was added to IssueTracker56Test unit test. Testing
showed no related JCK/JTREG failures.

With Best Regards,
Aleksei


Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

roger riggs
Hi Aleksei,

In the case of creating SAXException and then calling initCause() it
looks like
the value returned by getException() and getCause() may differ.
The past behavior and the descriptions of those two methods are the same.
Is the change intentional?

If not, you may need to override initCause() to update the 'exception'
field.

Roger


On 12/21/2017 12:27 PM, Aleks Efimov wrote:

> Hello,
>
> Please, help to review the fix for jaxp bug that fixes SAXException to
> correctly set exception cause with 'setCause' method:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
> I've tried to keep the fix miminal with respect to serial form of this
> API class, i.e. kept private 'exception' field.
>
> The new test case was added to IssueTracker56Test unit test. Testing
> showed no related JCK/JTREG failures.
>
> With Best Regards,
> Aleksei
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Jason Mehrens
In reply to this post by Aleks Efimov
Aleksei,

You have to override all of the constructors to always disable initCause.  Actually a similar issue was covered in:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html 

Pretty much everything was hashed out in those threads.

Here is the final webrev for that:

http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html

That should be a good cookbook for changes and tests required for this issue.  Note that it gets rid of the Exception field and maintains serial compatibility.  
Looking back on that change, I would have changed it so the InvalidClassException added the double cause as suppressed exceptions.

Jason

________________________________________
From: core-libs-dev <[hidden email]> on behalf of Aleks Efimov <[hidden email]>
Sent: Thursday, December 21, 2017 11:27 AM
To: core-libs-dev
Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Hello,

Please, help to review the fix for jaxp bug that fixes SAXException to
correctly set exception cause with 'setCause' method:
http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
I've tried to keep the fix miminal with respect to serial form of this
API class, i.e. kept private 'exception' field.

The new test case was added to IssueTracker56Test unit test. Testing
showed no related JCK/JTREG failures.

With Best Regards,
Aleksei


Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Aleks Efimov
Hi Roger, Jason,

I've tried to avoid doing the same stuff as I did for XPathException to
minimize the changes to the SAXException class. But I was naive to think so:
Tried to keep the exception field in place to avoid modifications
required to keep serial form intact (similar as it was done in
JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
(spotted by Roger, thank you!) that can make cause/exception values
inconsistent. To avoid the API modification and for the sake of clarity
I proceeded with removal of the 'exception' field. The new version of
the fix:
- Removes 'exception' field
- Maintains the serial form of the SAXException class
- Handles the difference in SAXException:exception and Throwable:cause
types 'SAXException::getException', i.e. SAXException:exception is
Exception typed and Throwable:cause is Throwable typed.
- Avoids any changes to API and serial form of the class, but
maintaining it similar to JDK-8009581)
- There is a copy of SAXException class in java.base module that is
required for j.u.Properties::loadFromXML, it has been update with the
same changes.

New regression test has been added to test:
- Serial compatibility with legacy JDKs (similar to JDK-8009581)
- usages of SAXException::initCause/getException

Webrev with the fix and new regression:
http://cr.openjdk.java.net/~aefimov/6857903/dev/01/

Testing shows no JCK/regression tests failures with the proposed fix.

Best Regards,
Aleksei


On 12/21/2017 07:00 PM, Jason Mehrens wrote:

> Aleksei,
>
> You have to override all of the constructors to always disable initCause.  Actually a similar issue was covered in:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>
> Pretty much everything was hashed out in those threads.
>
> Here is the final webrev for that:
>
> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>
> That should be a good cookbook for changes and tests required for this issue.  Note that it gets rid of the Exception field and maintains serial compatibility.
> Looking back on that change, I would have changed it so the InvalidClassException added the double cause as suppressed exceptions.
>
> Jason
>
> ________________________________________
> From: core-libs-dev <[hidden email]> on behalf of Aleks Efimov <[hidden email]>
> Sent: Thursday, December 21, 2017 11:27 AM
> To: core-libs-dev
> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>
> Hello,
>
> Please, help to review the fix for jaxp bug that fixes SAXException to
> correctly set exception cause with 'setCause' method:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
> I've tried to keep the fix miminal with respect to serial form of this
> API class, i.e. kept private 'exception' field.
>
> The new test case was added to IssueTracker56Test unit test. Testing
> showed no related JCK/JTREG failures.
>
> With Best Regards,
> Aleksei
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Jason Mehrens
Aleksei,

Looks good.  We should avoid this.initCause in readObject and prefer super.initCause so subclasses can't alter the deserialization behavior.
While I can't think of a case off the top of my head, I would prefer that we trap and convert IllegalStateException into a InvalidClassException in readObject.
That keeps the readObject contract intact in case something malicious is going on.  That is just my preference though.

Jason
________________________________________
From: Aleks Efimov <[hidden email]>
Sent: Monday, February 5, 2018 10:51 AM
To: Jason Mehrens; 'Roger Riggs'
Cc: core-libs-dev
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Hi Roger, Jason,

I've tried to avoid doing the same stuff as I did for XPathException to
minimize the changes to the SAXException class. But I was naive to think so:
Tried to keep the exception field in place to avoid modifications
required to keep serial form intact (similar as it was done in
JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
(spotted by Roger, thank you!) that can make cause/exception values
inconsistent. To avoid the API modification and for the sake of clarity
I proceeded with removal of the 'exception' field. The new version of
the fix:
- Removes 'exception' field
- Maintains the serial form of the SAXException class
- Handles the difference in SAXException:exception and Throwable:cause
types 'SAXException::getException', i.e. SAXException:exception is
Exception typed and Throwable:cause is Throwable typed.
- Avoids any changes to API and serial form of the class, but
maintaining it similar to JDK-8009581)
- There is a copy of SAXException class in java.base module that is
required for j.u.Properties::loadFromXML, it has been update with the
same changes.

New regression test has been added to test:
- Serial compatibility with legacy JDKs (similar to JDK-8009581)
- usages of SAXException::initCause/getException

Webrev with the fix and new regression:
http://cr.openjdk.java.net/~aefimov/6857903/dev/01/

Testing shows no JCK/regression tests failures with the proposed fix.

Best Regards,
Aleksei


On 12/21/2017 07:00 PM, Jason Mehrens wrote:

> Aleksei,
>
> You have to override all of the constructors to always disable initCause.  Actually a similar issue was covered in:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>
> Pretty much everything was hashed out in those threads.
>
> Here is the final webrev for that:
>
> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>
> That should be a good cookbook for changes and tests required for this issue.  Note that it gets rid of the Exception field and maintains serial compatibility.
> Looking back on that change, I would have changed it so the InvalidClassException added the double cause as suppressed exceptions.
>
> Jason
>
> ________________________________________
> From: core-libs-dev <[hidden email]> on behalf of Aleks Efimov <[hidden email]>
> Sent: Thursday, December 21, 2017 11:27 AM
> To: core-libs-dev
> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>
> Hello,
>
> Please, help to review the fix for jaxp bug that fixes SAXException to
> correctly set exception cause with 'setCause' method:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
> I've tried to keep the fix miminal with respect to serial form of this
> API class, i.e. kept private 'exception' field.
>
> The new test case was added to IssueTracker56Test unit test. Testing
> showed no related JCK/JTREG failures.
>
> With Best Regards,
> Aleksei
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Aleks Efimov
Thank you for your comments, Jason!
New webrev can be found at this location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/02/

The list of changes:
- this.initCause() -> super.initCause()
- 'readObject' has been modified to convert IllegalStateException to
InvalidClassException. The test case that triggers IllegalStateException
in SAXException::readObject has been added to
'testReadObjectIllegalStateException' test method.

Best Regards,
Aleksei

On 02/05/2018 05:09 PM, Jason Mehrens wrote:

> Aleksei,
>
> Looks good.  We should avoid this.initCause in readObject and prefer super.initCause so subclasses can't alter the deserialization behavior.
> While I can't think of a case off the top of my head, I would prefer that we trap and convert IllegalStateException into a InvalidClassException in readObject.
> That keeps the readObject contract intact in case something malicious is going on.  That is just my preference though.
>
> Jason
> ________________________________________
> From: Aleks Efimov <[hidden email]>
> Sent: Monday, February 5, 2018 10:51 AM
> To: Jason Mehrens; 'Roger Riggs'
> Cc: core-libs-dev
> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>
> Hi Roger, Jason,
>
> I've tried to avoid doing the same stuff as I did for XPathException to
> minimize the changes to the SAXException class. But I was naive to think so:
> Tried to keep the exception field in place to avoid modifications
> required to keep serial form intact (similar as it was done in
> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
> (spotted by Roger, thank you!) that can make cause/exception values
> inconsistent. To avoid the API modification and for the sake of clarity
> I proceeded with removal of the 'exception' field. The new version of
> the fix:
> - Removes 'exception' field
> - Maintains the serial form of the SAXException class
> - Handles the difference in SAXException:exception and Throwable:cause
> types 'SAXException::getException', i.e. SAXException:exception is
> Exception typed and Throwable:cause is Throwable typed.
> - Avoids any changes to API and serial form of the class, but
> maintaining it similar to JDK-8009581)
> - There is a copy of SAXException class in java.base module that is
> required for j.u.Properties::loadFromXML, it has been update with the
> same changes.
>
> New regression test has been added to test:
> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
> - usages of SAXException::initCause/getException
>
> Webrev with the fix and new regression:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>
> Testing shows no JCK/regression tests failures with the proposed fix.
>
> Best Regards,
> Aleksei
>
>
> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>> Aleksei,
>>
>> You have to override all of the constructors to always disable initCause.  Actually a similar issue was covered in:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>>
>> Pretty much everything was hashed out in those threads.
>>
>> Here is the final webrev for that:
>>
>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>>
>> That should be a good cookbook for changes and tests required for this issue.  Note that it gets rid of the Exception field and maintains serial compatibility.
>> Looking back on that change, I would have changed it so the InvalidClassException added the double cause as suppressed exceptions.
>>
>> Jason
>>
>> ________________________________________
>> From: core-libs-dev <[hidden email]> on behalf of Aleks Efimov <[hidden email]>
>> Sent: Thursday, December 21, 2017 11:27 AM
>> To: core-libs-dev
>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>>
>> Hello,
>>
>> Please, help to review the fix for jaxp bug that fixes SAXException to
>> correctly set exception cause with 'setCause' method:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>> I've tried to keep the fix miminal with respect to serial form of this
>> API class, i.e. kept private 'exception' field.
>>
>> The new test case was added to IssueTracker56Test unit test. Testing
>> showed no related JCK/JTREG failures.
>>
>> With Best Regards,
>> Aleksei
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Jason Mehrens
Aleksei,

I missed this before but, it looks like we are calling the public getException method in writeObject.  We should create a private method to service both the public getException method and writeObject to guard against subclass tampering.

Everything else looks good.

Jason
________________________________________
From: Aleks Efimov <[hidden email]>
Sent: Monday, February 5, 2018 8:46 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Thank you for your comments, Jason!
New webrev can be found at this location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/02/

The list of changes:
- this.initCause() -> super.initCause()
- 'readObject' has been modified to convert IllegalStateException to
InvalidClassException. The test case that triggers IllegalStateException
in SAXException::readObject has been added to
'testReadObjectIllegalStateException' test method.

Best Regards,
Aleksei

On 02/05/2018 05:09 PM, Jason Mehrens wrote:

> Aleksei,
>
> Looks good.  We should avoid this.initCause in readObject and prefer super.initCause so subclasses can't alter the deserialization behavior.
> While I can't think of a case off the top of my head, I would prefer that we trap and convert IllegalStateException into a InvalidClassException in readObject.
> That keeps the readObject contract intact in case something malicious is going on.  That is just my preference though.
>
> Jason
> ________________________________________
> From: Aleks Efimov <[hidden email]>
> Sent: Monday, February 5, 2018 10:51 AM
> To: Jason Mehrens; 'Roger Riggs'
> Cc: core-libs-dev
> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>
> Hi Roger, Jason,
>
> I've tried to avoid doing the same stuff as I did for XPathException to
> minimize the changes to the SAXException class. But I was naive to think so:
> Tried to keep the exception field in place to avoid modifications
> required to keep serial form intact (similar as it was done in
> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
> (spotted by Roger, thank you!) that can make cause/exception values
> inconsistent. To avoid the API modification and for the sake of clarity
> I proceeded with removal of the 'exception' field. The new version of
> the fix:
> - Removes 'exception' field
> - Maintains the serial form of the SAXException class
> - Handles the difference in SAXException:exception and Throwable:cause
> types 'SAXException::getException', i.e. SAXException:exception is
> Exception typed and Throwable:cause is Throwable typed.
> - Avoids any changes to API and serial form of the class, but
> maintaining it similar to JDK-8009581)
> - There is a copy of SAXException class in java.base module that is
> required for j.u.Properties::loadFromXML, it has been update with the
> same changes.
>
> New regression test has been added to test:
> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
> - usages of SAXException::initCause/getException
>
> Webrev with the fix and new regression:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>
> Testing shows no JCK/regression tests failures with the proposed fix.
>
> Best Regards,
> Aleksei
>
>
> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>> Aleksei,
>>
>> You have to override all of the constructors to always disable initCause.  Actually a similar issue was covered in:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>>
>> Pretty much everything was hashed out in those threads.
>>
>> Here is the final webrev for that:
>>
>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>>
>> That should be a good cookbook for changes and tests required for this issue.  Note that it gets rid of the Exception field and maintains serial compatibility.
>> Looking back on that change, I would have changed it so the InvalidClassException added the double cause as suppressed exceptions.
>>
>> Jason
>>
>> ________________________________________
>> From: core-libs-dev <[hidden email]> on behalf of Aleks Efimov <[hidden email]>
>> Sent: Thursday, December 21, 2017 11:27 AM
>> To: core-libs-dev
>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>>
>> Hello,
>>
>> Please, help to review the fix for jaxp bug that fixes SAXException to
>> correctly set exception cause with 'setCause' method:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>> I've tried to keep the fix miminal with respect to serial form of this
>> API class, i.e. kept private 'exception' field.
>>
>> The new test case was added to IssueTracker56Test unit test. Testing
>> showed no related JCK/JTREG failures.
>>
>> With Best Regards,
>> Aleksei
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

roger riggs
In reply to this post by Aleks Efimov
Hi Aleksei,

SAXException.java x 2, SAXExceptionInitCause:
   Please use explicit imports (not java.io.*).

SAXEXception.java:145: I would have expected:
if (cause instanceof Exception) {...}

SAXExceptionInitCause.java:
   Great to see all the test cases.

   Generally, we recommend using try-with-resources but in this case it
does not add much because
   exceptions can't occur when using BAOS.

Thanks, Roger

On 2/5/2018 9:46 PM, Aleks Efimov wrote:

> Thank you for your comments, Jason!
> New webrev can be found at this location:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/02/
>
> The list of changes:
> - this.initCause() -> super.initCause()
> - 'readObject' has been modified to convert IllegalStateException to
> InvalidClassException. The test case that triggers
> IllegalStateException in SAXException::readObject has been added to
> 'testReadObjectIllegalStateException' test method.
>
> Best Regards,
> Aleksei
>
> On 02/05/2018 05:09 PM, Jason Mehrens wrote:
>> Aleksei,
>>
>> Looks good.  We should avoid this.initCause in readObject and prefer
>> super.initCause so subclasses can't alter the deserialization behavior.
>> While I can't think of a case off the top of my head, I would prefer
>> that we trap and convert IllegalStateException into a
>> InvalidClassException in readObject.
>> That keeps the readObject contract intact in case something malicious
>> is going on.  That is just my preference though.
>>
>> Jason
>> ________________________________________
>> From: Aleks Efimov <[hidden email]>
>> Sent: Monday, February 5, 2018 10:51 AM
>> To: Jason Mehrens; 'Roger Riggs'
>> Cc: core-libs-dev
>> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>> not correctly set Exception
>>
>> Hi Roger, Jason,
>>
>> I've tried to avoid doing the same stuff as I did for XPathException to
>> minimize the changes to the SAXException class. But I was naive to
>> think so:
>> Tried to keep the exception field in place to avoid modifications
>> required to keep serial form intact (similar as it was done in
>> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
>> (spotted by Roger, thank you!) that can make cause/exception values
>> inconsistent. To avoid the API modification and for the sake of clarity
>> I proceeded with removal of the 'exception' field. The new version of
>> the fix:
>> - Removes 'exception' field
>> - Maintains the serial form of the SAXException class
>> - Handles the difference in SAXException:exception and Throwable:cause
>> types 'SAXException::getException', i.e. SAXException:exception is
>> Exception typed and Throwable:cause is Throwable typed.
>> - Avoids any changes to API and serial form of the class, but
>> maintaining it similar to JDK-8009581)
>> - There is a copy of SAXException class in java.base module that is
>> required for j.u.Properties::loadFromXML, it has been update with the
>> same changes.
>>
>> New regression test has been added to test:
>> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
>> - usages of SAXException::initCause/getException
>>
>> Webrev with the fix and new regression:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>>
>> Testing shows no JCK/regression tests failures with the proposed fix.
>>
>> Best Regards,
>> Aleksei
>>
>>
>> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>>> Aleksei,
>>>
>>> You have to override all of the constructors to always disable
>>> initCause.  Actually a similar issue was covered in:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html 
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html 
>>>
>>>
>>> Pretty much everything was hashed out in those threads.
>>>
>>> Here is the final webrev for that:
>>>
>>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html 
>>>
>>>
>>> That should be a good cookbook for changes and tests required for
>>> this issue.  Note that it gets rid of the Exception field and
>>> maintains serial compatibility.
>>> Looking back on that change, I would have changed it so the
>>> InvalidClassException added the double cause as suppressed exceptions.
>>>
>>> Jason
>>>
>>> ________________________________________
>>> From: core-libs-dev <[hidden email]> on
>>> behalf of Aleks Efimov <[hidden email]>
>>> Sent: Thursday, December 21, 2017 11:27 AM
>>> To: core-libs-dev
>>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not
>>> correctly set Exception
>>>
>>> Hello,
>>>
>>> Please, help to review the fix for jaxp bug that fixes SAXException to
>>> correctly set exception cause with 'setCause' method:
>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>>> I've tried to keep the fix miminal with respect to serial form of this
>>> API class, i.e. kept private 'exception' field.
>>>
>>> The new test case was added to IssueTracker56Test unit test. Testing
>>> showed no related JCK/JTREG failures.
>>>
>>> With Best Regards,
>>> Aleksei
>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Aleks Efimov
Hi Jason, Roger,

Thanks for your comments.
The new webrev with suggested/recommended edits can be viewed at this
location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/03/

Thanks,
Aleksei

PS: Also configured my IDE to use explicit imports =)

On 02/06/2018 03:31 PM, Roger Riggs wrote:

> Hi Aleksei,
>
> SAXException.java x 2, SAXExceptionInitCause:
>   Please use explicit imports (not java.io.*).
>
> SAXEXception.java:145: I would have expected:
> if (cause instanceof Exception) {...}
>
> SAXExceptionInitCause.java:
>   Great to see all the test cases.
>
>   Generally, we recommend using try-with-resources but in this case it
> does not add much because
>   exceptions can't occur when using BAOS.
>
> Thanks, Roger
>
> On 2/5/2018 9:46 PM, Aleks Efimov wrote:
>> Thank you for your comments, Jason!
>> New webrev can be found at this location:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/02/
>>
>> The list of changes:
>> - this.initCause() -> super.initCause()
>> - 'readObject' has been modified to convert IllegalStateException to
>> InvalidClassException. The test case that triggers
>> IllegalStateException in SAXException::readObject has been added to
>> 'testReadObjectIllegalStateException' test method.
>>
>> Best Regards,
>> Aleksei
>>
>> On 02/05/2018 05:09 PM, Jason Mehrens wrote:
>>> Aleksei,
>>>
>>> Looks good.  We should avoid this.initCause in readObject and prefer
>>> super.initCause so subclasses can't alter the deserialization behavior.
>>> While I can't think of a case off the top of my head, I would prefer
>>> that we trap and convert IllegalStateException into a
>>> InvalidClassException in readObject.
>>> That keeps the readObject contract intact in case something
>>> malicious is going on.  That is just my preference though.
>>>
>>> Jason
>>> ________________________________________
>>> From: Aleks Efimov <[hidden email]>
>>> Sent: Monday, February 5, 2018 10:51 AM
>>> To: Jason Mehrens; 'Roger Riggs'
>>> Cc: core-libs-dev
>>> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>> not correctly set Exception
>>>
>>> Hi Roger, Jason,
>>>
>>> I've tried to avoid doing the same stuff as I did for XPathException to
>>> minimize the changes to the SAXException class. But I was naive to
>>> think so:
>>> Tried to keep the exception field in place to avoid modifications
>>> required to keep serial form intact (similar as it was done in
>>> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause'
>>> method
>>> (spotted by Roger, thank you!) that can make cause/exception values
>>> inconsistent. To avoid the API modification and for the sake of clarity
>>> I proceeded with removal of the 'exception' field. The new version of
>>> the fix:
>>> - Removes 'exception' field
>>> - Maintains the serial form of the SAXException class
>>> - Handles the difference in SAXException:exception and Throwable:cause
>>> types 'SAXException::getException', i.e. SAXException:exception is
>>> Exception typed and Throwable:cause is Throwable typed.
>>> - Avoids any changes to API and serial form of the class, but
>>> maintaining it similar to JDK-8009581)
>>> - There is a copy of SAXException class in java.base module that is
>>> required for j.u.Properties::loadFromXML, it has been update with the
>>> same changes.
>>>
>>> New regression test has been added to test:
>>> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
>>> - usages of SAXException::initCause/getException
>>>
>>> Webrev with the fix and new regression:
>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>>>
>>> Testing shows no JCK/regression tests failures with the proposed fix.
>>>
>>> Best Regards,
>>> Aleksei
>>>
>>>
>>> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>>>> Aleksei,
>>>>
>>>> You have to override all of the constructors to always disable
>>>> initCause.  Actually a similar issue was covered in:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html 
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html 
>>>>
>>>>
>>>> Pretty much everything was hashed out in those threads.
>>>>
>>>> Here is the final webrev for that:
>>>>
>>>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html 
>>>>
>>>>
>>>> That should be a good cookbook for changes and tests required for
>>>> this issue.  Note that it gets rid of the Exception field and
>>>> maintains serial compatibility.
>>>> Looking back on that change, I would have changed it so the
>>>> InvalidClassException added the double cause as suppressed exceptions.
>>>>
>>>> Jason
>>>>
>>>> ________________________________________
>>>> From: core-libs-dev <[hidden email]> on
>>>> behalf of Aleks Efimov <[hidden email]>
>>>> Sent: Thursday, December 21, 2017 11:27 AM
>>>> To: core-libs-dev
>>>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>>> not correctly set Exception
>>>>
>>>> Hello,
>>>>
>>>> Please, help to review the fix for jaxp bug that fixes SAXException to
>>>> correctly set exception cause with 'setCause' method:
>>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>>>> I've tried to keep the fix miminal with respect to serial form of this
>>>> API class, i.e. kept private 'exception' field.
>>>>
>>>> The new test case was added to IssueTracker56Test unit test. Testing
>>>> showed no related JCK/JTREG failures.
>>>>
>>>> With Best Regards,
>>>> Aleksei
>>>>
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Jason Mehrens
Aleksei,

Looks good to me.

Jason
________________________________________
From: Aleks Efimov <[hidden email]>
Sent: Wednesday, February 7, 2018 7:24 PM
To: Roger Riggs; Jason Mehrens
Cc: [hidden email]
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

Hi Jason, Roger,

Thanks for your comments.
The new webrev with suggested/recommended edits can be viewed at this
location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/03/

Thanks,
Aleksei

PS: Also configured my IDE to use explicit imports =)

On 02/06/2018 03:31 PM, Roger Riggs wrote:

> Hi Aleksei,
>
> SAXException.java x 2, SAXExceptionInitCause:
>   Please use explicit imports (not java.io.*).
>
> SAXEXception.java:145: I would have expected:
> if (cause instanceof Exception) {...}
>
> SAXExceptionInitCause.java:
>   Great to see all the test cases.
>
>   Generally, we recommend using try-with-resources but in this case it
> does not add much because
>   exceptions can't occur when using BAOS.
>
> Thanks, Roger
>
> On 2/5/2018 9:46 PM, Aleks Efimov wrote:
>> Thank you for your comments, Jason!
>> New webrev can be found at this location:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/02/
>>
>> The list of changes:
>> - this.initCause() -> super.initCause()
>> - 'readObject' has been modified to convert IllegalStateException to
>> InvalidClassException. The test case that triggers
>> IllegalStateException in SAXException::readObject has been added to
>> 'testReadObjectIllegalStateException' test method.
>>
>> Best Regards,
>> Aleksei
>>
>> On 02/05/2018 05:09 PM, Jason Mehrens wrote:
>>> Aleksei,
>>>
>>> Looks good.  We should avoid this.initCause in readObject and prefer
>>> super.initCause so subclasses can't alter the deserialization behavior.
>>> While I can't think of a case off the top of my head, I would prefer
>>> that we trap and convert IllegalStateException into a
>>> InvalidClassException in readObject.
>>> That keeps the readObject contract intact in case something
>>> malicious is going on.  That is just my preference though.
>>>
>>> Jason
>>> ________________________________________
>>> From: Aleks Efimov <[hidden email]>
>>> Sent: Monday, February 5, 2018 10:51 AM
>>> To: Jason Mehrens; 'Roger Riggs'
>>> Cc: core-libs-dev
>>> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>> not correctly set Exception
>>>
>>> Hi Roger, Jason,
>>>
>>> I've tried to avoid doing the same stuff as I did for XPathException to
>>> minimize the changes to the SAXException class. But I was naive to
>>> think so:
>>> Tried to keep the exception field in place to avoid modifications
>>> required to keep serial form intact (similar as it was done in
>>> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause'
>>> method
>>> (spotted by Roger, thank you!) that can make cause/exception values
>>> inconsistent. To avoid the API modification and for the sake of clarity
>>> I proceeded with removal of the 'exception' field. The new version of
>>> the fix:
>>> - Removes 'exception' field
>>> - Maintains the serial form of the SAXException class
>>> - Handles the difference in SAXException:exception and Throwable:cause
>>> types 'SAXException::getException', i.e. SAXException:exception is
>>> Exception typed and Throwable:cause is Throwable typed.
>>> - Avoids any changes to API and serial form of the class, but
>>> maintaining it similar to JDK-8009581)
>>> - There is a copy of SAXException class in java.base module that is
>>> required for j.u.Properties::loadFromXML, it has been update with the
>>> same changes.
>>>
>>> New regression test has been added to test:
>>> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
>>> - usages of SAXException::initCause/getException
>>>
>>> Webrev with the fix and new regression:
>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>>>
>>> Testing shows no JCK/regression tests failures with the proposed fix.
>>>
>>> Best Regards,
>>> Aleksei
>>>
>>>
>>> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>>>> Aleksei,
>>>>
>>>> You have to override all of the constructors to always disable
>>>> initCause.  Actually a similar issue was covered in:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>>>>
>>>>
>>>> Pretty much everything was hashed out in those threads.
>>>>
>>>> Here is the final webrev for that:
>>>>
>>>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>>>>
>>>>
>>>> That should be a good cookbook for changes and tests required for
>>>> this issue.  Note that it gets rid of the Exception field and
>>>> maintains serial compatibility.
>>>> Looking back on that change, I would have changed it so the
>>>> InvalidClassException added the double cause as suppressed exceptions.
>>>>
>>>> Jason
>>>>
>>>> ________________________________________
>>>> From: core-libs-dev <[hidden email]> on
>>>> behalf of Aleks Efimov <[hidden email]>
>>>> Sent: Thursday, December 21, 2017 11:27 AM
>>>> To: core-libs-dev
>>>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>>> not correctly set Exception
>>>>
>>>> Hello,
>>>>
>>>> Please, help to review the fix for jaxp bug that fixes SAXException to
>>>> correctly set exception cause with 'setCause' method:
>>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>>>> I've tried to keep the fix miminal with respect to serial form of this
>>>> API class, i.e. kept private 'exception' field.
>>>>
>>>> The new test case was added to IssueTracker56Test unit test. Testing
>>>> showed no related JCK/JTREG failures.
>>>>
>>>> With Best Regards,
>>>> Aleksei
>>>>
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

roger riggs
Looks fine  +1

Thanks, Roger


On 2/8/2018 10:13 AM, Jason Mehrens wrote:

> Aleksei,
>
> Looks good to me.
>
> Jason
> ________________________________________
> From: Aleks Efimov <[hidden email]>
> Sent: Wednesday, February 7, 2018 7:24 PM
> To: Roger Riggs; Jason Mehrens
> Cc: [hidden email]
> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>
> Hi Jason, Roger,
>
> Thanks for your comments.
> The new webrev with suggested/recommended edits can be viewed at this
> location:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/03/
>
> Thanks,
> Aleksei
>
> PS: Also configured my IDE to use explicit imports =)
>
> On 02/06/2018 03:31 PM, Roger Riggs wrote:
>> Hi Aleksei,
>>
>> SAXException.java x 2, SAXExceptionInitCause:
>>    Please use explicit imports (not java.io.*).
>>
>> SAXEXception.java:145: I would have expected:
>> if (cause instanceof Exception) {...}
>>
>> SAXExceptionInitCause.java:
>>    Great to see all the test cases.
>>
>>    Generally, we recommend using try-with-resources but in this case it
>> does not add much because
>>    exceptions can't occur when using BAOS.
>>
>> Thanks, Roger
>>
>> On 2/5/2018 9:46 PM, Aleks Efimov wrote:
>>> Thank you for your comments, Jason!
>>> New webrev can be found at this location:
>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/02/
>>>
>>> The list of changes:
>>> - this.initCause() -> super.initCause()
>>> - 'readObject' has been modified to convert IllegalStateException to
>>> InvalidClassException. The test case that triggers
>>> IllegalStateException in SAXException::readObject has been added to
>>> 'testReadObjectIllegalStateException' test method.
>>>
>>> Best Regards,
>>> Aleksei
>>>
>>> On 02/05/2018 05:09 PM, Jason Mehrens wrote:
>>>> Aleksei,
>>>>
>>>> Looks good.  We should avoid this.initCause in readObject and prefer
>>>> super.initCause so subclasses can't alter the deserialization behavior.
>>>> While I can't think of a case off the top of my head, I would prefer
>>>> that we trap and convert IllegalStateException into a
>>>> InvalidClassException in readObject.
>>>> That keeps the readObject contract intact in case something
>>>> malicious is going on.  That is just my preference though.
>>>>
>>>> Jason
>>>> ________________________________________
>>>> From: Aleks Efimov <[hidden email]>
>>>> Sent: Monday, February 5, 2018 10:51 AM
>>>> To: Jason Mehrens; 'Roger Riggs'
>>>> Cc: core-libs-dev
>>>> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>>> not correctly set Exception
>>>>
>>>> Hi Roger, Jason,
>>>>
>>>> I've tried to avoid doing the same stuff as I did for XPathException to
>>>> minimize the changes to the SAXException class. But I was naive to
>>>> think so:
>>>> Tried to keep the exception field in place to avoid modifications
>>>> required to keep serial form intact (similar as it was done in
>>>> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause'
>>>> method
>>>> (spotted by Roger, thank you!) that can make cause/exception values
>>>> inconsistent. To avoid the API modification and for the sake of clarity
>>>> I proceeded with removal of the 'exception' field. The new version of
>>>> the fix:
>>>> - Removes 'exception' field
>>>> - Maintains the serial form of the SAXException class
>>>> - Handles the difference in SAXException:exception and Throwable:cause
>>>> types 'SAXException::getException', i.e. SAXException:exception is
>>>> Exception typed and Throwable:cause is Throwable typed.
>>>> - Avoids any changes to API and serial form of the class, but
>>>> maintaining it similar to JDK-8009581)
>>>> - There is a copy of SAXException class in java.base module that is
>>>> required for j.u.Properties::loadFromXML, it has been update with the
>>>> same changes.
>>>>
>>>> New regression test has been added to test:
>>>> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
>>>> - usages of SAXException::initCause/getException
>>>>
>>>> Webrev with the fix and new regression:
>>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>>>>
>>>> Testing shows no JCK/regression tests failures with the proposed fix.
>>>>
>>>> Best Regards,
>>>> Aleksei
>>>>
>>>>
>>>> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>>>>> Aleksei,
>>>>>
>>>>> You have to override all of the constructors to always disable
>>>>> initCause.  Actually a similar issue was covered in:
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>>>>>
>>>>>
>>>>> Pretty much everything was hashed out in those threads.
>>>>>
>>>>> Here is the final webrev for that:
>>>>>
>>>>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>>>>>
>>>>>
>>>>> That should be a good cookbook for changes and tests required for
>>>>> this issue.  Note that it gets rid of the Exception field and
>>>>> maintains serial compatibility.
>>>>> Looking back on that change, I would have changed it so the
>>>>> InvalidClassException added the double cause as suppressed exceptions.
>>>>>
>>>>> Jason
>>>>>
>>>>> ________________________________________
>>>>> From: core-libs-dev <[hidden email]> on
>>>>> behalf of Aleks Efimov <[hidden email]>
>>>>> Sent: Thursday, December 21, 2017 11:27 AM
>>>>> To: core-libs-dev
>>>>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>>>> not correctly set Exception
>>>>>
>>>>> Hello,
>>>>>
>>>>> Please, help to review the fix for jaxp bug that fixes SAXException to
>>>>> correctly set exception cause with 'setCause' method:
>>>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>>>>> I've tried to keep the fix miminal with respect to serial form of this
>>>>> API class, i.e. kept private 'exception' field.
>>>>>
>>>>> The new test case was added to IssueTracker56Test unit test. Testing
>>>>> showed no related JCK/JTREG failures.
>>>>>
>>>>> With Best Regards,
>>>>> Aleksei
>>>>>
>>>>>