Repository navigation
Json Arithmetic operation resulted in an overflow #609
Description
Activity
- addeduntriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner
on Dec 6, 2019 The exception comes from
int newSize = checked(_buffer.Length + growBy); This particular exception (at least within the JSON serializer code) comes from the internal
PooledByteBufferWriter:
runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PooledByteBufferWriter.cs
Line 141 in 57e5755
int newSize = checked(_rentedBuffer.Length + growBy); Proposal:
- make the exception and message more informative in Json code. ("Result exceeded maximum buffer size (2 Gb?)")
- make the exception and message more informative in System.Buffers code too.
Both of those seem reasonable to me. Adding an if-check on the size that throws makes sense (though I don't know if, in some contexts, checking/throwing might introduce noticeable overhead). That said, it is possible that we have other edge cases where
checkedarithmetic is used to guard against integer overflow or OOM when asking for buffers that won't fit in aT[]or sizes that thearraypoolcan't honor.Anyone interested in a PR with tests?
- addedhelp wanted[up-for-grabs] Good issue for external contributors[up-for-grabs] Good issue for external contributorsenhancementProduct code improvement that does NOT require public API changes/additionsProduct code improvement that does NOT require public API changes/additionsgood first issueIssue should be easy to implement, good for first-time contributorsIssue should be easy to implement, good for first-time contributorsand removeduntriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner
on Dec 9, 2019 @ahsonkhan I'm interested, could you assign me?
Reacted by Ilya and Dan MoseleyReacted by Ahson Khan, Dan Moseley and Nate Barbettini- removedhelp wanted[up-for-grabs] Good issue for external contributors[up-for-grabs] Good issue for external contributors
on Dec 24, 2019 @felipepessoto done.
@ahsonkhan currently the checked keyword only validates the integer overflow (if the value would exceed int.MaxValue).
But ArrayPool.Shared.Rent won't work for int.MaxValue anyway, because we can't allocate new byte[] of int.MaxValue size (I believe that is because of array headers overhead). Throwing another non-user-friendly exception in rare occasions where the newSize is equal to int.MaxSize or very close to it.In this case, should we validate if the newSize is smaller than (int.MaxValue - 56) (for 64 bit process)?
For 32bits processes, I guess the header is a bit smaller. But it will generate an OutOfMemoryException anyway, unless we set the LARGEADDRESSAWARE flag (I haven't tested this scenario)because we can't allocate new byte[] of int.MaxValue size (I believe that is because of array headers overhead)
It's a bit more complicated than headers overhead.
https://docs.microsoft.com/en-us/dotnet/api/system.array?view=netcore-3.1
The array size is limited to a total of 4 billion elements, and to a maximum index of 0X7FEFFFFF in any given dimension (0X7FFFFFC7 for byte arrays and arrays of single-byte structures).
Nice, I didn't know that. So we can compare it to Array.MaxByteArrayLength or 0X7FFFFFC7
2 remaining items
So we can compare it to Array.MaxByteArrayLength or 0X7FFFFFC7
Using
MaxByteArrayLengthwould be better but AFAIK it's internal to corelib and this code seems to be in another assembly.Actually, just clamping the result to
int.MaxValuemight be enough - if you attempt to allocate anint.MaxValuelength array the runtime will generate:System.OutOfMemoryException: Array dimensions exceeded supported range.which seems like a reasonable error to me.
@mikedn, if we let the array constructor throw the OutOfMemoryException, we'll be back to the original problem where the Exception message is not clear. I could validate the size and throw an OutOfMemoryException with a custom message.
Stephen also gave a suggestion here: #1308 (comment)
we'll be back to the original problem where the Exception message is not clear
I don't think so, the original message is "Arithmetic operation resulted in an overflow." which is indeed not very clear, it's not obvious to anyone what the cause of the problem is. The "Array dimensions exceeded supported range" message should be obvious enough. Unless you really want to tell the user - "Hey, your JSON is yugeeee!" - so there's definitely no confusion :)
Besides, this looks very much like a corner case. The fact that a 2GB JSON is first generated in memory and then written to a file seems pretty dubious to me.
we'll be back to the original problem where the Exception message is not clear
I don't think so, the original message is "Arithmetic operation resulted in an overflow." which is indeed not very clear, it's not obvious to anyone what the cause of the problem is. The "Array dimensions exceeded supported range" message should be obvious enough. Unless you really want to tell the user - "Hey, your JSON is yugeeee!" - so there's definitely no confusion :)
Besides, this looks very much like a corner case. The fact that a 2GB JSON is first generated in memory and then written to a file seems pretty dubious to me.
You are right. Agreed
Re-opening until #32587 is done.
- added a commit that references this issue
on Mar 20, 2020 Re-opening until #34040 is done.
- ghost locked as resolved and limited conversation to collaborators
on Dec 11, 2020
Experimenting with new Json API in PowerShell Core repo I get very large output and as result exception "Arithmetic operation resulted in an overflow".
The exception message looks non-user-friendly and non-useful.
Proposal:
Additional information
The exception comes from
runtime/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs
Line 177 in 00813df
Stack trace from PowerShell:
PowerShell can be downloaded from the PR PowerShell/PowerShell#11198
Repo scripts: