On Tue, 2 May 2023 13:26:31 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. > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Throw StreamCorruptedException instead of InvalidClassException and handle > negative array size in checkArray() as well `StreamCorruptedException` looks better. I have a few nitpicks. test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 46: > 44: private static byte[] buildArrayPayload() throws IOException { > 45: // Serialize to bytes > 46: ByteArrayOutputStream baos = new ByteArrayOutputStream(); Suggestion: private static byte[] buildArrayPayload() throws IOException { // Serialize to bytes ByteArrayOutputStream baos = new ByteArrayOutputStream(); test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 106: > 104: > 105: public static void main(String[] args) throws Exception { > 106: try { Test code nits, we can probably `try-with-resources` this thing to make it cleaner and make sure nothing unexpectedly bad is happening on "close" paths. ------------- Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13540#pullrequestreview-1409425435 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1182757260 PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1182813958
