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

[simonis]

This issue was reported by: Yakov Shafranovich (yakovsh@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 NegativeArraySizeExceptions. 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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8306744 to be approved

Issues

• JDK-8306461: ObjectInputStream::readObject() should handle negative array sizes without throwing NegativeArraySizeExceptions
• JDK-8306744: ObjectInputStream::readObject() should handle negative array sizes without throwing NegativeArraySizeExceptions (CSR)

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 4762023a
• Roger Riggs (@RogerRiggs - Reviewer)

Contributors

• Yakov Shafranovich <yakovsh@amazon.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13540/head:pull/13540
$ git checkout pull/13540

Update a local copy of the PR:
$ git checkout pull/13540
$ git pull https://git.openjdk.org/jdk.git pull/13540/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13540

View PR using the GUI difftool:
$ git pr show -t 13540

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13540.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back simonis! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@simonis The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[simonis]

/contributor add Yakov Shafranovich yakovsh@amazon.com

[openjdk]

@simonis
Contributor Yakov Shafranovich <yakovsh@amazon.com> successfully added.

[mlbridge]

Webrevs

• 05: Full - Incremental (da07109e)
• 04: Full - Incremental (e351073e)
• 03: Full - Incremental (4762023a)
• 02: Full - Incremental (4bc579e6)
• 01: Full - Incremental (f242dca2)
• 00: Full (d68ceaab)

[jddarcy]

/csr required

[jddarcy]

Please file a CSR for the proposed behavioral change.

[openjdk]

@jddarcy usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

[jddarcy]

/csr needed

[openjdk]

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@simonis please create a CSR request for issue JDK-8306461 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[simonis]

Hi @jddarcy,

I'm happy to create a CSR for this change, but I'm a little bit unsure about the details. From my understanding this qualifies as a behavioral change, right? But this behavior wasn't specified before at all. Neither did the API specification of ObjectInputStream::readObject() mention that it can throw a NegativeArraySizeException nor did the Serialization Specification mentioned the case of a negative array size.

Previously, readObject() could throw a NegativeArraySizeException which will now, with this PR, be changed into a InvalidClassException. Do you agree that this is an implementation detail and therefor the CSR should have "Implementation" Scope?

Thanks,
Volker

[shipilev]

Some suggestions

[jddarcy]

Hi @jddarcy,

I'm happy to create a CSR for this change, but I'm a little bit unsure about the details. From my understanding this qualifies as a behavioral change, right? But this behavior wasn't specified before at all. Neither did the API specification of ObjectInputStream::readObject() mention that it can throw a NegativeArraySizeException nor did the Serialization Specification mentioned the case of a negative array size.

Previously, readObject() could throw a NegativeArraySizeException which will now, with this PR, be changed into a InvalidClassException. Do you agree that this is an implementation detail and therefor the CSR should have "Implementation" Scope?

Thanks, Volker

Hi Volker,

Yes, the behavior wasn't specified, but that doesn't imply users haven't become reliant on it, hence the (behavioral) compatibility review via a CSR of the implementation change.

HTH

[simonis]

Hi @turbanoff, @shipilev and @RogerRiggs,

Thanks for your reviews so far. I've hopefully addressed them all. I've also created a CSR for the issue:

https://bugs.openjdk.org/browse/JDK-8306744

Please feel free to review it :)

[shipilev]

Test questions:

[shipilev]

Also, merge from master to get Windows GHA fixed.

[shipilev]

Looks fine to me.

[RogerRiggs]

A bit more investigation is needed.
I noticed that ObjectInputStream.checkArray throws NegativeArraySizeException if length is < 0 before calling filterCheck.
The checkArray method is called via SharedSecrets to check array sizes on some of the collection classes during deserialization.
Likely, the change should be extended to cover negative lengths in those cases too.

[simonis]

Hi @RogerRiggs ,

I've checked the callers of checkArray() and found 13, all of them in the corresponding readObject() methods:

class	NASE	exception
j.u.IdentityHashMap	+	StreamCorruptedException("Illegal mappings count: " + size)
j.u.HashMap	+	InvalidObjectException("Illegal mappings count: " + mappings)
j.u.Properties	+	StreamCorruptedException("Illegal # of Elements: " + elements)
j.u.ArrayList	+	InvalidObjectException("Invalid size: " + size)
j.u.ImmutableCollections	+	InvalidObjectException("negative length " + len)
j.u.Hashtable	+	StreamCorruptedException("Illegal # of Elements: " + elements)
j.u.HashSet	+	InvalidObjectException("Illegal capacity: " + capacity)
jx.m.o.TabularDataSupport	(+)	(depends on ArrayList)
j.u.concurrent/PriorityBlockingQueue	(-)	(depends on PriorityQueue)
j.u.PriorityQueue	-
j.u.ArrayDeque	-
j.u.concurrent/CopyOnWriteArrayList	-
j.u.Collections	-

The occurrences with a "+" int he "NASE" column all already handle the negative array size case before calling ObjectInputStream.checkArray() and throw the exceptions listed in the "exception" column if the array size is negative.

So instead of changing ObjectInputStream.checkArray() maybe it makes sense to fix the remaining four classes (i.e. PriorityQueue, ArrayDeque, CopyOnWriteArrayList and Collections) to handle negative array sizes in their corresponding readObject() methods as well? This somehow feels more natural to me, because, other then within the initial issue, this is not a problem of the deserialization protocol, but rather an issue with custom deserialization code in the the readObject() methods. Therefor I also think it would make sense to handle that as a different issue. What do you think?

[RogerRiggs]

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.

[simonis]

Hi @RogerRiggs,

I have now updated both, checkArray() and readArray() to throw a StreamCorruptedException("Array length is negative") in the case of a negative array size. I've also changed the signature of ObjectInputStream::checkArray() and JavaObjectInputStreamAccess::checkArray to throw an ObjectStreamException instead of an InvalidClassException because ObjectStreamException is the super class of both InvalidClassException and StreamCorruptedException. Finally I've updated the test with an extra case of a PriorityQueue with a negative element size in order to test checkArray().

OK now? Once we read consensus here I'll update the CSR accordingly.

[RogerRiggs]

Look good, thanks for fixing checkArray().

[shipilev]

StreamCorruptedException looks better. I have a few nitpicks.

[openjdk]

@simonis This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Co-authored-by: Yakov Shafranovich <yakovsh@amazon.com>
Reviewed-by: shade, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 169 new commits pushed to the master branch:

• bb3e44d: 8307603: [AIX] Broken build after JDK-8307301
• 5c7ede9: 8307347: serviceability/sa/ClhsdbDumpclass.java could leave files owned by root on macOS
• 89b7d07: 8307100: Remove ReferentBasedDiscovery reference discovery policy
• f6ea897: 8306285: Missing file in search test
• 33245d6: 8307517: Add VMErrorCallback infrastructure to extend hs_err dumping
• 7a1cb64: 8293547: Add relaxed add_and_fetch for macos aarch64 atomics
• 68f385c: 8307428: jstat tests doesn't tolerate dash in the O column
• 8bbd264: 8307423: [s390x] Represent Registers as values
• 959e62c: 8307521: Introduce check_oop infrastructure to check oops in the oop class
• ad0e5a9: 8304720: SuperWord::schedule should rebuild C2-graph from SuperWord dependency-graph
• ... and 159 more: https://git.openjdk.org/jdk/compare/35e7bc21d3c1b38e2268924b20ae4b149b4f8cd8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[simonis]

I've created JDK-8307621 to track the release note for this issue.

[simonis]

/integrate

[openjdk]

Going to push as commit 4116b10.
Since your change was applied there have been 178 commits pushed to the master branch:

• 93ee19f: 8305486: Add split() variants that keep the delimiters to String and j.u.r.Pattern
• ad90fb6: 8293786: HttpClient will not send more than 64 kb of data from the 2nd request in http2
• 5a259d8: 8303153: Native interpreter frame missing mirror
• 4b02956: 8054022: HttpURLConnection timeouts with Expect: 100-Continue and no chunking
• 9f34e4f: 8304434: [AIX] Update minimum xlclang version
• d2e0e53: 8307604: gcc12 based Alpine build broken build after JDK-8307301
• e91f0d3: 8307571: Remove unused SomeConstants in WatcherThread class
• 26755a9: 8306408: Fix the format of several tables in building.md
• 64c0962: 8307569: Build with gcc8 is broken after JDK-8307301
• bb3e44d: 8307603: [AIX] Broken build after JDK-8307301
• ... and 168 more: https://git.openjdk.org/jdk/compare/35e7bc21d3c1b38e2268924b20ae4b149b4f8cd8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@simonis Pushed as commit 4116b10.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.