Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/6.0-staging] Fix JsonDocument thread safety. #92832

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Sep 29, 2023

Backport of #76716 to release/6.0-staging

/cc @eiriktsarpalis

Customer Impact

Even though the JsonDocument/JsonElement types are documented as immutable/thread safe values, the current implementation suffers from a concurrency bug that can result in incorrect strings being returned when multiple threads attempt to call the GetString() method on the same document. This can result in nondeterministic bugs including potential privacy or security problems.

Testing

Added an OuterLoop test validating the absence of the problem.

Risk

Low. This PR removes the cache that was causing the issue. We have concluded that the removal of the cache should not contribute to any perf regressions in real-life scenaria.

@ghost
Copy link

ghost commented Sep 29, 2023

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

Backport of #76716 to release/6.0-staging

/cc @eiriktsarpalis

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis force-pushed the jsondocument-thread-safety-6.0 branch from dadadd6 to fadec73 Compare September 29, 2023 18:59
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Sep 29, 2023
@eiriktsarpalis eiriktsarpalis added this to the 6.0.x milestone Sep 29, 2023
@eiriktsarpalis eiriktsarpalis requested review from carlossanlop and ericstj and removed request for steveharter September 29, 2023 19:12
@ericstj ericstj added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 30, 2023
@ericstj ericstj merged commit f95e178 into dotnet:release/6.0-staging Sep 30, 2023
126 of 129 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to backport JsonDocument thread safety fix to System.Text.Json v7 (PR#76716)
2 participants