RFR 8009736: Comparator API cleanup

[Henry Jen]

On 06/14/2013 03:07 AM, Paul Sandoz wrote:
> 
> On Jun 13, 2013, at 7:28 PM, Tom Hawtin <tom.hawtin at oracle.com> wrote:
> 
>> On 12/06/2013 17:36, Paul Sandoz wrote:
>>
>>> "The returned comparator is serializable and does not permit null arguments when comparing objects".
>>
>> Being Serializable at all is odd. It brings the previously 
>> implementation-dependent name and fields into the API spec.
>>
> 
> That is, alas, inherently the nature of serialization. Collections are serializable, some collections take Comparator instances on construction (e.g. TreeSet).
> 
> 
>> Serialisability also allows curious behaviour. Whilst not, say, a 
>> vulnerability itself, oddities aren't generally a good idea. For an 
>> example of strangeness, NullComparator can have a sign of 
>> -Integer.MIN_VALUE. It can also have a `real` field of null which is 
>> otherwise checked for during construction,
> 
> I dunno what to say except "serialization is bad! i wish it were never in the JDK" :-)
> 
> 
>> although strangely the 
>> compare method also checks whether real is null or not.
>>
> 
> Yes, Henry is gonna fix that null check.
> 
> FWIW i find this marginally clearer:
> 
>         NullComparator(boolean nullsFirst, Comparator<? super T> real) {
>             this(nullsFirst ? -1 : 1, real);
>         }
> 
>         private NullComparator(int sign, Comparator<? super T> real) {
>             this.sign = sign;
>             this.real = (Comparator<T>) Objects.requireNonNull(real);
>         }
> 

I was thinking to change sign to null to work out the -Integer.MIN_VALUE
issue from serialization tampering.

As null real, that's NPE at runtime, not a security threat, so I'll
probably let it pass without validating at de-serialization time. Unless
we feel it's important enough to fail-fast.

Cheers,
Henry