On Wed, 19 Apr 2023 16:47:33 GMT, Volker Simonis <simo...@openjdk.org> wrote:
> This issue was reported by: Yakov Shafranovich > ([yako...@amazon.com](mailto:yako...@amazon.com)) > > Currently, `ObjectInputStream::readObject()` doesn't explicitly checks for a > negative array length in the deserialization stream. Instead it calls > `j.l.r.Array::newInstance(..)` with the negative length which results in a > `NegativeArraySizeException`. NegativeArraySizeException is an unchecked > exception which is neither declared in the signature of > `ObjectInputStream::readObject()` nor mentioned in its API specification. It > is therefore not obvious for users of `ObjectInputStream::readObject()` that > they may have to handle `NegativeArraySizeException`s. It would therefor be > better if a negative array length in the deserialization stream would be > automatically wrapped in an `InvalidClassException` which is a checked > exception (derived from `IOException` via `ObjectStreamException`) and > declared in the signature of `ObjectInputStream::readObject()`. > > If we do the negative array length check in `ObjectInputStream::readObject()` > before filtering, this will then also fix > `ObjectInputFilter.FilterInfo::arrayLength()` which is defined as: > > Returns: > the non-negative number of array elements when deserializing an array of the > class, otherwise -1 > > but currently returns a negative value if the array length is negative. Some suggestions src/java.base/share/classes/java/io/ObjectInputStream.java line 2142: > 2140: int len = bin.readInt(); > 2141: if (len < 0) { > 2142: throw new InvalidClassException(desc.getName(), "Array > length < 0 (" + len + ")"); Suggestion: throw new InvalidClassException(desc.getName(), "Array length (" + len + ") is negative"); test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 43: > 41: public class NegativeArraySizeTest { > 42: > 43: private static byte[] _buildPayload() throws IOException { `_buildPayload` -> `buildPayload`. test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 44: > 42: > 43: private static byte[] _buildPayload() throws IOException { > 44: String[] simpleArray = new String[1]; Inline this variable? test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 55: > 53: // Find the right location to modify, looking for the first > instance of TC_ENDBLOCKDATA > 54: int firstPos = 0; > 55: for (int i=0; i<serializedData.length-1; i++) { Suggestion: for (int i = 0; i < serializedData.length - 1; i++) { test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 95: > 93: public static void main(String[] args) throws Exception { > 94: try { > 95: test_simpleArray_negative(); Inline these `test_*` methods. Move the `CustomFilter` as test class nested. test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 99: > 97: throw new Exception("ObjectInputStream::readObject() > shouldn't throw a NegativeArraySizeException", nase); > 98: } catch (ObjectStreamException ose) { > 99: // OK, because a NegativeArraySizeException should be > converted into a ObjectStreamException Check the message from `ose` here? test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 109: > 107: throw new Exception("ObjectInputStream::readObject() > should catch NegativeArraySizeExceptions before filtering", ice); > 108: } > 109: // OK, because a NegativeArraySizeException should be > converted into a ObjectStreamException *before* filtering Sounds like we should really test for ICE with "Array length ... is negative" here. This would cover the case when filter rejected the class before that exception is thrown. ------------- PR Review: https://git.openjdk.org/jdk/pull/13540#pullrequestreview-1393736956 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172423915 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172424270 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172424783 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172425065 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172465910 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172467222 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1172482373
