RFR: 8306461: ObjectInputStream::readObject() should handle negative array sizes without throwing NegativeArraySizeExceptions [v4]

[Aleksey Shipilev]

On Tue, 2 May 2023 13:26:31 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> This issue was reported by: Yakov Shafranovich ([yakovsh at amazon.com](mailto:yakovsh at 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