Code review request for 6990094 "ObjectInputStream cloneArray doesn't handle short[]"

[Peter Jones]

Joe,

On Dec 3, 2010, at 11:06 AM, Joe Darcy wrote:
> Please review this simple fix for
> 
>   6990094 "ObjectInputStream cloneArray doesn't handle short[]"
>   http://cr.openjdk.java.net/~darcy/6990094.0/
> 
> The complete patch is
> 
> --- old/src/share/classes/java/io/ObjectInputStream.java    2010-12-03 00:31:24.000000000 -0800
> +++ new/src/share/classes/java/io/ObjectInputStream.java    2010-12-03 00:31:24.000000000 -0800
> @@ -3498,8 +3498,8 @@
>            return ((int[]) array).clone();
>        } else if (array instanceof long[]) {
>            return ((long[]) array).clone();
> -        } else if (array instanceof double[]) {
> -            return ((double[]) array).clone();
> +        } else if (array instanceof short[]) {
> +            return ((short[]) array).clone();
>        } else {
>            throw new AssertionError();
>        }

Ouch, I recall reviewing this code when it was added, but evidently missed this.  FWIW, the change looks good to me [peterjones].

> You'll notice there is no regression test for this change.  One justification is that the fix is in the "obviously no bugs" category. [1] There is an if-else instanceof chain over Object arrays and arrays of each primitive type; there are two checks for double and none for short so changing the one of the double checks to short is "obviously correct."

Agreed.

> Also, I've taken a stab a writing an explicit regression test for this condition, but the problem only manifests in cases beyond my direct serialization experience where a class has overridden the readUnshared method.

I think that there was a test to go along with the bug fix that added this code (although evidently it didn't cover the short[] case).  I don't see it in the hg repository now, I wonder if that's because the bug had the security keyword.

Regards,
-- Peter