Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Brian, Looks fine to me. Thanks, Roger On 4/15/2015 2:41 PM, Brian Burkhalter wrote: Updated and hopefully final patch here: http://cr.openjdk.java.net/~bpb/8075156/webrev.02/ Test run in progress. Barring any objections I assume that changing CODE_POINT_U from a String to an int does not require another Reviewer approval, so I will go ahead and push the fix after the yet to be refreshed CCC request is approved, if it is. Thanks, Brian On Apr 15, 2015, at 10:08 AM, Brian Burkhalter wrote: But I'm curious why CODE_POINT_U000 is defined as String instead of char? The indexOf() operation is more efficient for single characters. Probably because I had initially used contains(CharSequence) instead of indexOf(String) and when I changed the former to the latter I did not change the constant. I will modify this per your suggestion.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Updated and hopefully final patch here: http://cr.openjdk.java.net/~bpb/8075156/webrev.02/ Test run in progress. Barring any objections I assume that changing CODE_POINT_U from a String to an int does not require another Reviewer approval, so I will go ahead and push the fix after the yet to be refreshed CCC request is approved, if it is. Thanks, Brian On Apr 15, 2015, at 10:08 AM, Brian Burkhalter wrote: >> But I'm curious why CODE_POINT_U000 is defined as String instead of char? >> The indexOf() operation is more efficient for single characters. > > Probably because I had initially used contains(CharSequence) instead of > indexOf(String) and when I changed the former to the latter I did not change > the constant. I will modify this per your suggestion.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Roger, On Apr 15, 2015, at 9:50 AM, Roger Riggs wrote: > Looks fine as is. Thanks for reviewing it. > But I'm curious why CODE_POINT_U000 is defined as String instead of char? > The indexOf() operation is more efficient for single characters. Probably because I had initially used contains(CharSequence) instead of indexOf(String) and when I changed the former to the latter I did not change the constant. I will modify this per your suggestion. Thanks, Brian
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Brian, Looks fine as is. But I'm curious why CODE_POINT_U000 is defined as String instead of char? The indexOf() operation is more efficient for single characters. Thanks, Roger On 4/14/2015 5:56 PM, Brian Burkhalter wrote: Please review at your convenience this latest patch modified from the previous one pursuant to the most recent comments. Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.01/ Summary: * Revise Preferences javadoc to indicate IAE for put*(), get*(), and remove() for NUL control character U+ in the key and / or value, as appropriate. * Extend fix of put*() in JDK-8068373 to all platforms by moving functional code up to to AbstractPreferences. * Add fix for get*() and remove() in AbstractPreferences. * Revert FileSystemPreferences changes from JDK-8068373 as these are now handled by AbstractPreferences (note this includes rolling back the more recent year in the copyright as well). * Minor picayune cleanup in WindowsPreferences. * Correct class javadoc of AbstractPreferences which had referred to flush() and sync() as returning boolean type. * Extend existing test to get() and remove() cases. Test passed on all the usual platforms. The CCC request will need to be revised or a new one filed, pending approval of this patch. Thanks, Brian On Apr 14, 2015, at 8:37 AM, Brian Burkhalter wrote: So barring objections to the contrary from some other quarter, I will clean up the current patch and also address the errors in the class level javadoc that I pointed out in AbstractPreferences.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Please review at your convenience this latest patch modified from the previous one pursuant to the most recent comments. Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.01/ Summary: * Revise Preferences javadoc to indicate IAE for put*(), get*(), and remove() for NUL control character U+ in the key and / or value, as appropriate. * Extend fix of put*() in JDK-8068373 to all platforms by moving functional code up to to AbstractPreferences. * Add fix for get*() and remove() in AbstractPreferences. * Revert FileSystemPreferences changes from JDK-8068373 as these are now handled by AbstractPreferences (note this includes rolling back the more recent year in the copyright as well). * Minor picayune cleanup in WindowsPreferences. * Correct class javadoc of AbstractPreferences which had referred to flush() and sync() as returning boolean type. * Extend existing test to get() and remove() cases. Test passed on all the usual platforms. The CCC request will need to be revised or a new one filed, pending approval of this patch. Thanks, Brian On Apr 14, 2015, at 8:37 AM, Brian Burkhalter wrote: > So barring objections to the contrary from some other quarter, I will clean > up the current patch and also address the errors in the class level javadoc > that I pointed out in AbstractPreferences.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Roger, Thanks for taking the time to review this recap. Your suggestion to forge ahead with the approach already taken seems to be reasonable. I concur that a fail-fast approach of throwing an IAE as soon as the bad character is encountered if preferable to waiting for some inscrutable subsequent exception in flush() or sync(). So barring objections to the contrary from some other quarter, I will clean up the current patch and also address the errors in the class level javadoc that I pointed out in AbstractPreferences. Thanks, Brian On Apr 14, 2015, at 8:00 AM, Roger Riggs wrote: > Thanks for digging deeper and the recap. > > I don't see any cases in which it is necessary or valuable to allow > \0 in Strings (key or value). The original bug report did not indicate > whether > it was discovered as a testing exercise or when diagnosing a bug in an > application. > The compatibility risk is unknown at the moment since no cases have been > noted that require \0 in strings. > > Long term the code will be easier to maintain if it is less complex and has > fewer > variables that are platform or format specific and developer errors are > detected > as soon as possible. > > Of the possible remedies below, #2 seems the most practical. But it will > further > hide the problem from developers since calling flush and sync is never > required > for correct operation: > ///"explicit //flush//invocation is //not//required upon termination to > ensure that pending updates are made persistent"/ > Flush and sync would be the only opportunity to throw an exception and > explain the cause. > It will be harder to track down since the true cause will be far removed from > the time/location of the exception. > > In the absence of advice to continue to support \0in Preference strings on > Windows > or Mac I'd continue with the recent direction to make \0 in key and value > strings throw IAE.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Brian, Thanks for digging deeper and the recap. I don't see any cases in which it is necessary or valuable to allow \0 in Strings (key or value). The original bug report did not indicate whether it was discovered as a testing exercise or when diagnosing a bug in an application. The compatibility risk is unknown at the moment since no cases have been noted that require \0 in strings. Long term the code will be easier to maintain if it is less complex and has fewer variables that are platform or format specific and developer errors are detected as soon as possible. Of the possible remedies below, #2 seems the most practical. But it will further hide the problem from developers since calling flush and sync is never required for correct operation: ///"explicit //flush//invocation is //not//required upon termination to ensure that pending updates are made persistent"/ Flush and sync would be the only opportunity to throw an exception and explain the cause. It will be harder to track down since the true cause will be far removed from the time/location of the exception. In the absence of advice to continue to support \0in Preference strings on Windows or Mac I'd continue with the recent direction to make \0 in key and value strings throw IAE. Roger On 4/10/2015 6:26 PM, Brian Burkhalter wrote: On Apr 4, 2015, at 12:53 PM, Alan Bateman wrote: On 24/03/2015 19:20, Brian Burkhalter wrote: Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.00/ This is a sequel to the resolved issue https://bugs.openjdk.java.net/browse/JDK-8068373, (prefs) FileSystemPreferences writes \0 to XML storage, causing loss of all preferences, wherein the code point U+, the null control character, was made illegal to use as a key in the generic Unix file system-based Preferences. The issue at hand extends disallowing U+ as a key in the put() method on Mac OS X and Windows, and also disallows this use to the remove() methods on these platforms and in the generic Unix file system-based Preferences. Use of U+ in the corresponding get() method has not been disallowed as this method returns a default value. If it would be preferable that the behavior of get() with respect to the key U+ were the same as for put() and remove() then this patch may be updated to that effect. Minimally then the putXXX methods should make it clear that they throw IAE for this case. This would be a javadoc only change because the implementation does this as a consequence of the original patch. Agreed. Actually I am not completely satisfied with the fix for https://bugs.openjdk.java.net/browse/JDK-8068373 so I went back over all the discussions and notes on the various ways to fix the problem trying to rethink whether there might be a better solution. The problem is not really with the Preferences APIs per se, but rather with the ability of the XML-based backing store to handle all characters which might be present in a String, in this particular case \u, but there are other possible problematic characters as well. Ideally, the fix would be to modify the XML backing store to handle all possible characters. This does not however appear to be possible without introducing an incompatibility when the XML backing store is shared between the set of newer versions of Java which would support storing all characters and the older versions which do not. There are ways to minimize the incompatibility but not apparently eliminate it and the additional complexity might not merit the effort. In the RFC thread on 8068373, it was concluded that it would be better simply to disallow \u for XML-backed Preferences. This change however introduced an incompatibility with non-XML backing stores which might or might not be able to handle \u. So to address this incompatibility (and to address the companion get/remove methods) the present RFR was introduced. The present change has the potential however to break Preferences implementations which have a backing store for which \u *is* legal. Note also that all Preferences implementations should be able to handle all Strings if there is no interaction with the backing store, even in the case of the XML backing store. Without a round trip via the XML backing store no error would be encountered. This suggests an alternative fix of allowing the illegal character to be used “live” but disallowed from being written to a backing store which cannot handle it. I raise these alternatives here because if any were preferable, then there is no point in going further in the current direction as one changeset reversion would already be needed. In the original discussion then it was just a question as to whether get/getXXX and remove should be consistent. If the get and remove methods will always behave as if the key doesn't exist (and return the defau
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
On Apr 4, 2015, at 12:53 PM, Alan Bateman wrote: > On 24/03/2015 19:20, Brian Burkhalter wrote: >> Please review at your convenience. >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 >> Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.00/ >> >> This is a sequel to the resolved issue >> https://bugs.openjdk.java.net/browse/JDK-8068373, (prefs) >> FileSystemPreferences writes \0 to XML storage, causing loss of all >> preferences, wherein the code point U+, the null control character, was >> made illegal to use as a key in the generic Unix file system-based >> Preferences. >> >> The issue at hand extends disallowing U+ as a key in the put() method on >> Mac OS X and Windows, and also disallows this use to the remove() methods on >> these platforms and in the generic Unix file system-based Preferences. >> >> Use of U+ in the corresponding get() method has not been disallowed as >> this method returns a default value. If it would be preferable that the >> behavior of get() with respect to the key U+ were the same as for put() >> and remove() then this patch may be updated to that effect. >> > Minimally then the putXXX methods should make it clear that they throw IAE > for this case. This would be a javadoc only change because the implementation > does this as a consequence of the original patch. Agreed. Actually I am not completely satisfied with the fix for https://bugs.openjdk.java.net/browse/JDK-8068373 so I went back over all the discussions and notes on the various ways to fix the problem trying to rethink whether there might be a better solution. The problem is not really with the Preferences APIs per se, but rather with the ability of the XML-based backing store to handle all characters which might be present in a String, in this particular case \u, but there are other possible problematic characters as well. Ideally, the fix would be to modify the XML backing store to handle all possible characters. This does not however appear to be possible without introducing an incompatibility when the XML backing store is shared between the set of newer versions of Java which would support storing all characters and the older versions which do not. There are ways to minimize the incompatibility but not apparently eliminate it and the additional complexity might not merit the effort. In the RFC thread on 8068373, it was concluded that it would be better simply to disallow \u for XML-backed Preferences. This change however introduced an incompatibility with non-XML backing stores which might or might not be able to handle \u. So to address this incompatibility (and to address the companion get/remove methods) the present RFR was introduced. The present change has the potential however to break Preferences implementations which have a backing store for which \u *is* legal. Note also that all Preferences implementations should be able to handle all Strings if there is no interaction with the backing store, even in the case of the XML backing store. Without a round trip via the XML backing store no error would be encountered. This suggests an alternative fix of allowing the illegal character to be used “live” but disallowed from being written to a backing store which cannot handle it. I raise these alternatives here because if any were preferable, then there is no point in going further in the current direction as one changeset reversion would already be needed. > In the original discussion then it was just a question as to whether > get/getXXX and remove should be consistent. If the get and remove methods > will always behave as if the key doesn't exist (and return the default value > in the case of get) then it shouldn't require a javadoc change. In the case of get(), for implementations based on AbstractPreferences, any exception thrown by getSpi() will in any case be caught and the default value returned. For remove() there might after all be no point in checking the key if the key is disallowed from being stored in the first place, so the method call would be a no-op and neither an implementation nor a javadoc change would be needed. > However I suspect it will require an implementation change as there may be > non-XML backing stores might that allow \0 in the key (hence get and remove > should actually do something). As noted above, it might not be correct to extend the checked-in fix for 8068373 to the non-XML backing store cases as is being proposed in this RFR and likewise not to modify remove() symmetrically. Rethinking this entire topic l am now inclined to suggest retracting the prior fix of 8068373 and instead take one of the following approaches: 1. Modify the XML backing store to be able to handle at least \0 if not other characters problematic for XML. As noted above this might be complicated and introduce inter-version incompatibilities. 2. Allow \0 to be in Strings at
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
On 24/03/2015 19:20, Brian Burkhalter wrote: Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.00/ This is a sequel to the resolved issue https://bugs.openjdk.java.net/browse/JDK-8068373, (prefs) FileSystemPreferences writes \0 to XML storage, causing loss of all preferences, wherein the code point U+, the null control character, was made illegal to use as a key in the generic Unix file system-based Preferences. The issue at hand extends disallowing U+ as a key in the put() method on Mac OS X and Windows, and also disallows this use to the remove() methods on these platforms and in the generic Unix file system-based Preferences. Use of U+ in the corresponding get() method has not been disallowed as this method returns a default value. If it would be preferable that the behavior of get() with respect to the key U+ were the same as for put() and remove() then this patch may be updated to that effect. Minimally then the putXXX methods should make it clear that they throw IAE for this case. This would be a javadoc only change because the implementation does this as a consequence of the original patch. In the original discussion then it was just a question as to whether get/getXXX and remove should be consistent. If the get and remove methods will always behave as if the key doesn't exist (and return the default value in the case of get) then it shouldn't require a javadoc change. However I suspect it will require an implementation change as there may be non-XML backing stores might that allow \0 in the key (hence get and remove should actually do something). -Alan.
