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

[Aleksey Shipilev]

On Wed, 19 Apr 2023 16:47:33 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.

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