JsonDocument.GetString implementation is not thread safe

[eiriktsarpalis]

Even though JsonDocument is nominally immutable (modulo disposability), the internal GetString implementation employs a caching scheme that is not thread safe:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs

Line 30 in 5c8aade

private (int, string?) _lastIndexAndString = (-1, null);

Since the cached field is a struct that cannot be written to or read from atomically. It might be that we could fix this in the code using a regular System.Tuple, thus ensuring atomic access to cached entries. Obviously this has the downside of incurring one additional allocation, but at least we can guarantee immutability (modulo disposability of course).

Related to #17975

cc @layomia @krwq

Originally posted by @eiriktsarpalis in dotnet/docs#20563 (comment)

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

    > however we tripped over the caching that is done via _lastIndexAndString (i.e., if multiple threads call GetString on an element at the same time, then they may get back the wrong value).

Looking at the code, I would guess that this can only happen because of torn reads/writes. It might be that we could fix this in the code using a regular System.Tuple, thus ensuring atomic access to cached entries. Obviously this has the downside of incurring one additional allocation, but at least we can guarantee immutability (modulo disposability of course).

Related to #17975

cc @layomia @krwq

Originally posted by @eiriktsarpalis in dotnet/docs#20563 (comment)

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

Related issues: #17975, #31911, #28711

[stephentoub]

Do we document JsonDocument as being thread-safe? We generally don't make any guarantees about thread-safety of instance members unless otherwise explicitly documented, and there are many places where instance members do non-thread-safe caching. What makes this one special enough to be worth adding cost?

And, what is this particular cache used for? Is it really common to ask for the same string at the same index multiple times in a row with no intervening other requests?

[eiriktsarpalis]

Do we document JsonDocument as being thread-safe?

We do, the original issue in docs was opened to account for that discrepancy. I created this separate issue to track potentially making the type thread safe in the future.

[rymeskar]

@stephentoub
Because of the possible cross-polination of values, should this fix be marked as security fix?

[stephentoub]

I don't believe that's warranted.