Code Review Request for 6578042

[Alan Bateman]

On 02/11/2011 19:59, David Holmes wrote:
>
> I'm not seeing a distinction in those two statements. Both expect to 
> return the property value for a given key; both assume a valid value 
> is a String. clearProperty throws ClassCastException if the assumption 
> doesn't hold; getProperty instead returns null.
The distinction is "value" vs. "string value", where the latter is 
interpreted by Darryl's patch to be the String representation. While 
that avoids the CCE, it is clearly inconsistent with System.getProperty 
that I see is also specified to return the "string value". So I think we 
have to dismiss this approach. I think this leaves two options:

1. Document existing behavior, meaning @throws CCE (several of 
Properties methods already have this).

2. Change the method so that it is consistent with getProperty and so 
returns null if the property value is not a String.

While option 2 is clearly a change then I think it's worth exploring to 
understand the impact. Properties has always had a note in its javadoc 
to warn users that it inherits from Hashtable and it has a note to 
"strongly discourage" inserting entries that have a key or value that is 
not a String. My guess (and this is purely a guess) is that 
System.clearProperty is not used very often and so the likelihood of 
someone abusing the system properties and using clearProperty is 
probably low. I see the bug on bugs.sun.com doesn't have any votes or 
comments which suggests that this one doesn't have an angry mob either.

Darryl - since you have decided to tackle this one then I would suggest 
looking into this further and coming back with a recommendation. I would 
also suggest that as part of the patch that the javadoc for 
System.getProperty and Properties.getProperty be clarified so that it's 
clear that they return null when the properties have been compromised.

-Alan.