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 <aleksej.efi...@oracle.com> 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 <aleksej.efi...@oracle.com> > 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 <core-libs-dev-boun...@openjdk.java.net> on behalf of >> Aleks Efimov <aleksej.efi...@oracle.com> >> 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 >> >>
