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
