Throw from CodeBase if assembly is in the bundle #40974

agocke · 2020-08-18T08:10:56Z

No description provided.

Dotnet-GitSync-Bot · 2020-08-18T08:10:58Z

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

agocke · 2020-08-18T08:11:24Z

@swaroop-sridhar It looks like I've somehow screwed up the QCall, but I'm not sure how. Advice?

am11 · 2020-08-18T12:59:26Z

@agocke, it requires a line after

runtime/src/coreclr/src/vm/ecalllist.h

Line 442 in e8c0a4c

QCFuncElement("GetCodeBase", AssemblyNative::GetCodeBase)

Wraith2 · 2020-08-18T15:37:08Z

Is there a way for users to determine that an assembly has been loaded from memory without trying to check codebase and handling this exception?

agocke · 2020-08-18T17:26:48Z

@Wraith2 If you only want to check whether an Assembly was loaded from memory, calling Assembly.Location and checking if it's an empty string is the best current way.

This API is specifically because I need to know if the file is embedded in a single-file bundle, not that it's just in memory.

agocke · 2020-08-18T17:41:47Z

To test this we'll need a test for single-file, since this case will only be hit in single-file apps now.

vitek-karas · 2020-08-18T18:54:52Z

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

+            get
+            {
+                var runtimeAssembly = this;
+                if (IsInBundle(new QCallAssembly(ref runtimeAssembly)))


Do we really need a new QCall for this?
We could either throw from native (I know, that's tedious) or we could return null from the GetCodeBase QCall. As far as I can tell the native implementation of GetCodeBase will never return null.

I just don't want to introduce a new QCall if we don't need one (And also so far the managed parts of runtime were not aware of bundles).

I agree that it is cleaner to do the check within GetCodeBase() in this case.
We may eventually require the IsBundle() QCall API, but can sidestep it for now.

vitek-karas · 2020-08-18T18:56:09Z

src/coreclr/src/vm/pefile.cpp

-    result.Set(GetEffectivePath());
+    if (GetILimage()->IsInBundle())
+    {
+        result.Set(SString::Empty());


Basically here we would return null. Currently it looks weird that we make the same check twice in a row.

I got lost here -- how would we return null? It looks like SString turns all null pointers into empty strings. Do I need to change the QCall signature?

The QCall returns string, so that should not be a problem. AssemblyNative::GetCodeBase (Which is the QCall) should basically call retString.Set(NULL) - we will likely need to move the bundle logic into this method (or somewhat change the PEAssembly::GetCodeBase to be able to return null).

I think the problem I'm hitting is

runtime/src/coreclr/src/utilcode/sstring.cpp

Lines 299 to 300 in 6072e4d

if (string == NULL || *string == 0)

Clear();

where calling result.Set(NULL) ends up using an empty string internally.

Any suggestions?

Do you actually need to handle the bundle case here? Can the entire fix be to just throw exception in AssemblyNative::GetCodeBase?

This code path is also called by GetAssemblyName, and I think that should still work. I think it's fine if we set the CodeBase to null or empty string in the resulting AssemblyName, but I think GetAssemblyName is still a perfectly reasonable call.

Ah, I see. I think what you have here is fine. I do not see a problem with IsInBundle being checked twice. Patterns like that happen all the time.

Alternatively I could change the GetCodeBase call to return something other than void and use that as a sentinel for whether or not the app is in a bundle vs simply loaded from memory.

Personally I kind of like this approach, but if the pattern is frowned upon in the runtime code I'll keep the second QCall

If it's too complicated I don't have a problem with the current implementation - it works. I was just trying to see if there's a simpler way...

I could change the GetCodeBase call to return something other than void and use that as a sentinel for whether or not the app is in a bundle vs simply loaded from memory.

Personally I kind of like this approach

I like it too, because it identifies the intention (ex: differentiates from retuning null/empty-string due to an unrelated bug).

vitek-karas · 2020-08-18T18:57:17Z

src/libraries/System.Reflection/tests/AssemblyTests.cs

+            var inMemBlob = File.ReadAllBytes(SourceTestAssemblyPath);
+            var asm = Assembly.Load(inMemBlob);
+#pragma warning disable SYSLIB0012
+            Assert.Throws<PlatformNotSupportedException>(() => asm.CodeBase);


For non-single-file this should not throw - right? I don't understand this test...

Test is wrong now. I originally was throwing from everyhting that was in-memory before I figured out we actually doc'd the behavior. I need to write a new test using single-file, but don't know where to write the test

Based on this comment I think this is a placeholder, and the single-file test is TBD.

But I think CodeBase_InMemory should check Assert.NotEmpty(asm.CodeBase); and the single-file test should be added to AppHost.Bundle tests.

I need to write a new test using single-file, but don't know where to write the test

@agocke I think the easiest way to add a single-file test for this is to follow the BundleProbe test in AppHost.Bundle.Tests.

ghost · 2020-08-18T22:33:19Z

Tagging subscribers to this area: @swaroop-sridhar, @agocke
See info in area-owners.md if you want to be subscribed.

swaroop-sridhar · 2020-08-19T19:36:05Z

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

    <value>Value was either too large or too small for a UInt64.</value>
  </data>
+  <data name="PlatformNotSupported_CodeBase" xml:space="preserve">
+    <value>CodeBase is not supported on assemblies loaded from memory.</value>


I think this is a bit confusing. For assemblies loaded from byte-arrays, CodeBase doesn't throw, but returns the caller's location. So, it is better to say assemblies loaded from single-file bundle.

swaroop-sridhar · 2020-08-19T19:40:12Z

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

+                if (IsInBundle(new QCallAssembly(ref runtimeAssembly)))
+                {
+                    // Not supported if the assembly was loaded from memory
+                    throw new PlatformNotSupportedException(SR.PlatformNotSupported_CodeBase);


I think PlatformNotSupportedException is not the best exception to throw here. I understand that .net native used this exception, and we may keep using it for that reason. Unlike .net native, it is difficult to conceptualize .net apps and .net single-file apps to be separate platforms. Probably a better choice would be InvalidOperationException.

Also, should we tag this PR as a breaking change, because Assembly.CodeBase couldn't throw before, and now it does.

I agree that PlatformNotSupportedException is not the right exception to throw here. I think it may be more appropriate to throw NotSupportedException instead of InvalidOperationException.

I think InvalidOperationException is appropriate for cases where the operation was valid on this instance but it is not valid anymore; or when the state of the instance can change for the operation to become valid.

It is not the case here. The Assembly instance will always throw, no state will affect it.

jkotas · 2020-08-20T07:11:29Z

src/coreclr/src/vm/assemblynative.cpp

    }

    retString.Set(codebase);
+    return ret;


return ret; should be after END_QCALL;

src/coreclr/src/vm/assemblynative.cpp

src/coreclr/src/vm/pefile.h

agocke · 2020-08-20T23:30:13Z

Hmm I seem to have broken a number of calls to the GetCodeBase method, but I can't figure out how. All the scenarios show the stack:

Fatal error. Internal CLR error. (0x80131506)
   at System.Reflection.RuntimeAssembly.GetCodeBase(System.Runtime.CompilerServices.QCallAssembly, System.Runtime.CompilerServices.StringHandleOnStack)
   at System.Reflection.RuntimeAssembly.GetCodeBase()

Any suggestions on debugging this?

jkotas · 2020-08-21T00:06:06Z

Any suggestions on debugging this?

You can download .dmp linked from the test log and matching artifacts from CI, and open it in windbg.

I have done just that and I see that it is a crash at:

 # ChildEBP RetAddr  
00 0b9ad3cc 721054b9 coreclr!BundleFileLocation::IsValid [F:\workspace\_work\1\s\src\coreclr\src\inc\bundle.h @ 34] 
01 (Inline) -------- coreclr!PEImage::IsInBundle+0x8 [F:\workspace\_work\1\s\src\coreclr\src\vm\peimage.inl @ 53] 
02 0b9ad3d8 72107232 coreclr!PEAssembly::GetCodeBase+0x13 [F:\workspace\_work\1\s\src\coreclr\src\vm\pefile.cpp @ 2144] 
03 0b9ad624 0875da2f coreclr!AssemblyNative::GetCodeBase+0x62 [F:\workspace\_work\1\s\src\coreclr\src\vm\assemblynative.cpp @ 576] 
04 0b9ad668 0875d9bf System_Private_CoreLib!ILStubClass.IL_STUB_PInvoke(System.Runtime.CompilerServices.QCallAssembly, System.Runtime.CompilerServices.StringHandleOnStack)+0x47
05 0b9ad690 0875d87d System_Private_CoreLib!System.Reflection.RuntimeAssembly.GetCodeBase()+0x57 [/_/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs @ 79] 
06 0b9ad6cc 0c068d98 System_Private_CoreLib!System.Reflection.RuntimeAssembly.GetName(Boolean)+0x2d [/_/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs @ 107] 
07 0b9ad6e0 0875d68f System_Private_CoreLib!System.Reflection.Emit.AssemblyBuilder.GetName(Boolean)+0x30 [/_/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs @ 484] 
08 0b9ad6ec 0c0666ad System_Private_CoreLib!System.Reflection.Assembly.GetName()+0x17 [/_/src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs @ 100] 
09 0b9ad74c 0c0664ca System_Reflection_Emit_Tests!System.Reflection.Emit.Tests.AssemblyTests.VerifyAssemblyBuilder(System.Reflection.Emit.AssemblyBuilder, System.Reflection.AssemblyName, System.Collections.Generic.IEnumerable`1<System.Reflection.Emit.CustomAttributeBuilder>)+0x85
0a 0b9ad768 720f970f System_Reflection_Emit_Tests!System.Reflection.Emit.Tests.AssemblyTests.DefineDynamicAssembly_AssemblyName_AssemblyBuilderAccess_CustomAttributeBuilder(System.Reflection.AssemblyName, System.Reflection.Emit.AssemblyBuilderAccess, System.Collections.Generic.IEnumerable`1<System.Reflection.Emit.CustomAttributeBuilder>)+0x4a

We are crashing because of coreclr!PEFile::GetILimage returned null. This can happen for Reflection.Emit assemblies.

elinor-fung · 2020-08-25T00:07:37Z

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

        public override AssemblyName GetName(bool copiedName)
        {
-            string? codeBase = GetCodeBase(copiedName);
+            string? codeBase = GetCodeBase();


Does this mean that if I have an Assembly asm loaded from the bundle, asm.CodeBase will throw, but asm.GetName().CodeBase will be null?

Yup. I couldn't see a good way of making the behavior match. I don't think there's anything wrong with fetching an AssemblyName, so I don't think that should throw, but since the AssemblyName doesn't have a pointer back to the Assembly there's no way to hook just the CodeBase API, aside from doing something drastic like subclassing AssemblyName just for this scenario.

Open to alternatives though.

Also adds test for Module.FullyQualifiedName (cherry picked from commit 0546bd5)

[release/5.0] Port API fixes for single file apps Backport of #41019 and #40974 to release/5.0

akoeplinger · 2020-08-27T07:59:03Z

@agocke I assume we should port this change to the Mono version of RuntimeAssembly.CodeBase too?

agocke · 2020-08-27T17:55:12Z

@akoeplinger I was under the impression that Mono didn't support single-file publishing. Was that wrong? Mono might want to change the behavior for AOT though.

akoeplinger · 2020-08-27T18:36:17Z

@agocke it doesn't now but it could in the future. I'm asking because we have kind of a similar situation with WASM/Blazor where the assembly doesn't exist on "disk" so I think it'd make sense to throw in that case too.

agocke requested a review from vitek-karas August 18, 2020 17:40

agocke marked this pull request as ready for review August 18, 2020 17:40

agocke marked this pull request as draft August 18, 2020 17:40

agocke closed this Aug 18, 2020

agocke reopened this Aug 18, 2020

vitek-karas reviewed Aug 18, 2020

View reviewed changes

jkotas added the area-Single-File label Aug 18, 2020

swaroop-sridhar mentioned this pull request Aug 19, 2020

Return host's file path when using Environment.GetCommandLineArgs()[0] in single-file apps #41019

Merged

swaroop-sridhar reviewed Aug 19, 2020

View reviewed changes

davmason mentioned this pull request Aug 20, 2020

Fix GetAppdomainStaticAddress for non-collectible assemblies #41076

Merged

jkotas reviewed Aug 20, 2020

View reviewed changes

src/coreclr/src/vm/assemblynative.cpp Outdated

}

retString.Set(codebase);

return ret;

Copy link

Member

jkotas Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return ret; should be after END_QCALL;

jkotas reviewed Aug 20, 2020

View reviewed changes

src/coreclr/src/vm/assemblynative.cpp Outdated Show resolved Hide resolved

jkotas reviewed Aug 20, 2020

View reviewed changes

src/coreclr/src/vm/pefile.h Outdated Show resolved Hide resolved

jkotas approved these changes Aug 20, 2020

View reviewed changes

jaredpar mentioned this pull request Aug 21, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

jkotas approved these changes Aug 21, 2020

View reviewed changes

agocke marked this pull request as ready for review August 21, 2020 18:07

agocke closed this Aug 21, 2020

agocke reopened this Aug 21, 2020

swaroop-sridhar approved these changes Aug 21, 2020

View reviewed changes

davmason closed this in #41076 Aug 24, 2020

jkotas reopened this Aug 24, 2020

elinor-fung reviewed Aug 25, 2020

View reviewed changes

agocke mentioned this pull request Aug 25, 2020

Add single-file test for Module.FullyQualifiedName #41312

Closed

jaredpar mentioned this pull request Aug 25, 2020

Errors installing the SDK during builds #34015

Closed

agocke added 2 commits August 25, 2020 10:37

Throw from CodeBase if assembly is in the bundle

3faf471

Add test for Module.FullyQualifiedName

76f6ce9

agocke force-pushed the throw-in-codebase branch from e0c34a7 to 76f6ce9 Compare August 25, 2020 17:37

vitek-karas approved these changes Aug 25, 2020

View reviewed changes

agocke merged commit 0546bd5 into dotnet:master Aug 25, 2020

agocke deleted the throw-in-codebase branch August 25, 2020 22:51

agocke added a commit that referenced this pull request Aug 26, 2020

Throw from CodeBase if assembly is in the bundle (#40974)

b17d683

Also adds test for Module.FullyQualifiedName (cherry picked from commit 0546bd5)

github-actions bot mentioned this pull request Aug 26, 2020

[release/5.0] Port API fixes for single file apps #41385

Merged

agocke added a commit that referenced this pull request Aug 26, 2020

Merge pull request #41385 from dotnet/backport/pr-41019-to-release/5.0

94e7aa7

[release/5.0] Port API fixes for single file apps Backport of #41019 and #40974 to release/5.0

ericstj added breaking-change Issue or PR that represents a breaking API or functional change over a previous release. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Sep 15, 2020

ericstj mentioned this pull request Sep 15, 2020

IsolatedStorage doesn't work from Single-File application in 5.0 #42265

Closed

This was referenced Sep 22, 2020

[.NetCore 3.x] [.Net 5 RTM] Single File for OpenMU target 3.1 app, launch failed #42570

Closed

[5.0.100-rc.2.20471.7] Single File for OpenMU/HandyControl/TuringTrader target 5.0 app, tests failed #42665

Closed

am11 mentioned this pull request Sep 25, 2020

Fix SingleFile regression in client configuration #42176

Merged

agocke removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 25, 2020

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Throw from CodeBase if assembly is in the bundle #40974

Throw from CodeBase if assembly is in the bundle #40974

Uh oh!

Conversation

agocke commented Aug 18, 2020

Uh oh!

Dotnet-GitSync-Bot commented Aug 18, 2020

Uh oh!

agocke commented Aug 18, 2020

Uh oh!

am11 commented Aug 18, 2020

Uh oh!

Wraith2 commented Aug 18, 2020

Uh oh!

agocke commented Aug 18, 2020

Uh oh!

agocke commented Aug 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

agocke commented Aug 20, 2020

Uh oh!

jkotas commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akoeplinger commented Aug 27, 2020

Uh oh!

agocke commented Aug 27, 2020

Uh oh!

akoeplinger commented Aug 27, 2020

agocke Aug 19, 2020 •

edited

Loading

jkotas Aug 19, 2020 •

edited

Loading

jkotas commented Aug 21, 2020 •

edited

Loading