-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix resource stream for collectible assemblies #22925
Conversation
The GetManifestResourceStream was returning a stream that didn't keep the underlying assembly alive. For collectible assemblies, that means that an assembly could be collected and the stream still kept, potentially reading garbage. The fix is to create a new stream type that stores a reference to the underlying assembly, thus ensuring the proper lifetime management.
cc: @marek-safar |
@@ -29,6 +29,16 @@ internal class RuntimeAssembly : Assembly | |||
|
|||
#endregion | |||
|
|||
private class ManifestResourceStream : UnmanagedMemoryStream | |||
{ | |||
private RuntimeAssembly _manifestAssembly; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about releasing the reference on explicit Dispose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit Dispose does not guarantee that nobody is accessing the memory anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, it'd have to invalidate the object state (maybe v2)
@@ -29,6 +29,16 @@ internal class RuntimeAssembly : Assembly | |||
|
|||
#endregion | |||
|
|||
private class ManifestResourceStream : UnmanagedMemoryStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be sealed
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please |
* Fix resource stream for collectible assemblies The GetManifestResourceStream was returning a stream that didn't keep the underlying assembly alive. For collectible assemblies, that means that an assembly could be collected and the stream still kept, potentially reading garbage. The fix is to create a new stream type that stores a reference to the underlying assembly, thus ensuring the proper lifetime management. * Make the new stream class sealed Commit migrated from dotnet/coreclr@452c40d
The GetManifestResourceStream was returning a stream that didn't keep
the underlying assembly alive. For collectible assemblies, that means
that an assembly could be collected and the stream still kept,
potentially reading garbage.
The fix is to create a new stream type that stores a reference to the
underlying assembly, thus ensuring the proper lifetime management.
I have also added a regression test that checks that the stream keeps the underlying assembly alive and it also checks that once the stream reference is gone, the assembly is collected.