On Wed, 26 Apr 2023 11:55:23 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' into JDK-8306461 > - Simplified exception message and fixed test to check the correct message > - Addresed review comments of @turbanoff, @shipilev and @RogerRiggs > - 8306461: ObjectInputStream::readObject() should handle negative array > sizes without throwing NegativeArraySizeExceptions Thanks for the investigation. On the question of the exception thrown IllegalObjectException vs StreamCorruptedExeception, I'd lean toward StreamCorruptedException, including for the current PR; as that is more indicative of the issue raised. As for addressing the existing uses of checkArray that would throw NAE, I would rather see a single fix in checkArray than adding code in multiple other places. A fix in checkArray would cover future uses as well as current uses. The existing code that is checking len < 0 before calling checkArray can continue to do so to maintain compatibility on the exception thrown. Though a change would be unlikely to break user code it is better to avoid that. (It might cause changes existing tests). Handling it in a separate PR may be reasonable but it too will require a CSR (change in behavior from throwning NAE to SCE) and the cause and behavior change are generally the same as the current PR. If handled in a single PR/CSR and release note will have a more coherent and singular explanation. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13540#issuecomment-1526506534
