Hi Peter.
Peter Jones wrote:
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.
Off-list, Alan found the a related closed test and Stuart and I have
developed an explicit test that tickles this bug:
http://cr.openjdk.java.net/~darcy/6990094.1/
I plan to shortly push the fix with the test, although the test might be
renamed or tweaked first,
Thanks,
-Joe
