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

[mono] Fix race during mono_image_storage_open #55201

Merged
merged 1 commit into from
Jul 14, 2021
Merged

[mono] Fix race during mono_image_storage_open #55201

merged 1 commit into from
Jul 14, 2021

Conversation

uweigand
Copy link
Contributor

@uweigand uweigand commented Jul 6, 2021

  • The mono_refcount_inc call in mono_image_storage_trypublish or
    mono_image_storage_tryaddref may abort when racing against a
    mono_image_storage_dtor that already decremented the refcount.

This race triggered in some cases when building aspnetcore using a Mono-based dotnet host SDK. The problem is that mono_image_storage_close runs outside the mono_images_storage_lock (and this may be unavoidable due to lock ordering concerns). Therefore, we can have a refcount that was already decremented to zero, but before mono_image_storage_dtor finishes removing the object from images_storage_hash, a parallel mono_image_storage_trypublish may have retrieved it from there. In that case, the mono_refcount_inc call will abort.

Fixed by detecting that case via mono_refcount_tryinc instead, and simply treating the object as if it had already been removed. It will in time actually get removed, either by the parallel mono_image_storage_dtor, or else by the g_hash_table_insert in mono_image_storage_trypublish (which will safely replace it with the new object, and mono_image_storage_dtor will then detect that and skip removal).

* The mono_refcount_inc call in mono_image_storage_trypublish or
  mono_image_storage_tryaddref may abort when racing against a
  mono_image_storage_dtor that already decremented the refcount.
@ghost
Copy link

ghost commented Jul 6, 2021

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details
  • The mono_refcount_inc call in mono_image_storage_trypublish or
    mono_image_storage_tryaddref may abort when racing against a
    mono_image_storage_dtor that already decremented the refcount.

This race triggered in some cases when building aspnetcore using a Mono-based dotnet host SDK. The problem is that mono_image_storage_close runs outside the mono_images_storage_lock (and this may be unavoidable due to lock ordering concerns). Therefore, we can have a refcount that was already decremented to zero, but before mono_image_storage_dtor finishes removing the object from images_storage_hash, a parallel mono_image_storage_trypublish may have retrieved it from there. In that case, the mono_refcount_inc call will abort.

Fixed by detecting that case via mono_refcount_tryinc instead, and simply treating the object as if it had already been removed. It will in time actually get removed, either by the parallel mono_image_storage_dtor, or else by the g_hash_table_insert in mono_image_storage_trypublish (which will safely replace it with the new object, and mono_image_storage_dtor will then detect that and skip removal).

Author: uweigand
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@lambdageek
Copy link
Member

/azp run sync-runtime-to-mono

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marek-safar marek-safar merged commit 7bd4724 into dotnet:main Jul 14, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 14, 2021
…debugger_custom_views

* 'main' of github.com:thaystg/runtime: (125 commits)
  [wasm] [debugger] Support method calls  (dotnet#55458)
  [debugger] Fix debugging after hot reloading (dotnet#55599)
  Inliner: Extend IL limit for profiled call-sites, allow inlining for switches. (dotnet#55478)
  DiagnosticSourceEventSource supports base class properties (dotnet#55613)
  [mono] Fix race during mono_image_storage_open (dotnet#55201)
  [mono] Add wrapper info for native func wrappers. (dotnet#55602)
  H/3 and Quic AppContext switch (dotnet#55332)
  Compression.ZipFile support for Unix Permissions (dotnet#55531)
  [mono] Fix skipping of static methods during IMT table construction. (dotnet#55610)
  Combine System.Private.Xml TrimmingTests projects (dotnet#55606)
  fix name conflict with Configuration class (dotnet#55597)
  Finish migrating RSAOpenSsl from RSA* to EVP_PKEY*
  Disable generic math (dotnet#55540)
  Obsolete CryptoConfig.EncodeOID (dotnet#55592)
  Address System.Net.Http.WinHttpHandler's nullable warnings targeting .NETCoreApp (dotnet#54995)
  Enable Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled (dotnet#55572)
  Fix Task.WhenAny failure mode when passed ICollection of zero tasks (dotnet#55580)
  Consume DistributedContextPropagator in DiagnosticsHandler (dotnet#55392)
  Add property ordering feature (dotnet#55586)
  Reduce subtest count in Reflection (dotnet#55537)
  ...
@uweigand uweigand deleted the mono-race-imagestorage branch July 14, 2021 12:20
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants