[9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key

[Brian Burkhalter]

On Apr 4, 2015, at 12:53 PM, Alan Bateman <Alan.Bateman at oracle.com> 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+0000, 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+0000 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+0000 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+0000 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 \u0000, 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 \u0000 for XML-backed Preferences. This change however introduced an incompatibility with non-XML backing stores which might or might not be able to handle \u0000. 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 \u0000 *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 the API level but do not write these entries to an XML backing store. This would allow in-session use of unsupported characters but these would magically disappear thereafter which could cause confusion.

3. Change the put() specification to allow throwing an IAE if the backing store cannot handle the supplied Strings. This however runs somewhat counter to the Preferences class-level specification that "The user of this class needn't be concerned with details of the backing store.”

On reexamining things I also noticed this paragraph in the AbstractPreferences class-level specification:

"The remaining SPI methods putSpi(String,String), removeSpi(String) and childSpi(String) have more complicated exception behavior. They are not specified to throw BackingStoreException, as they can generally obey their contracts even if the backing store is unavailable. This is true because they return no information and their effects are not required to become permanent until a subsequent call to Preferences.flush() or Preferences.sync(). Generally speaking, these SPI methods should not throw exceptions. In some implementations, there may be circumstances under which these calls cannot even enqueue the requested operation for later processing. Even under these circumstances it is generally better to simply ignore the invocation and return, rather than throwing an exception. Under these circumstances, however, all subsequent invocations of flush() and sync should return false, as returning true would imply that all previous operations had successfully been made permanent."

This suggests that solution #2 above might be the appropriate fix instead of that of 8068373 plus the patch in the current RFR. There are errors in the above specification paragraph however which should also be fixed, viz., flush{Spi}() and sync{Spi}() are void, not boolean-returning methods.

Thanks,

Brian