Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Enable AllowPartiallyTrustedCaller for DiagnosticSource on netfx #17076

Merged

Conversation

lmolkova
Copy link

This change marks DiagnosticSource with AllowPartiallyTrustedCaller on net46 and netfx to make it callable from partially trusted code.

E.g. System.Net.Http for .NET Framework declares AllowPartiallyTrustedCaller to be called by partially trusted code therefore can't have DiagnosticSource instrumentation.

Note that support for Activity on .NET 4.5 is dropped, so configuration is removed from the project.

@stephentoub
Copy link
Member

cc: @vancem, @morganbr, @ericstj

#else // To keep things short, we drop the operation name
string ret = s_uniqPrefix + "-" + Interlocked.Increment(ref s_currentRootId).ToString("x") + '.';
#endif
return ret;
}

[SecuritySafeCritical]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this attribute exist everywhere? (see build failure). Note that if it exists in .net Core 2.0 (which it probably does), I would not have a problem at all #ifdefing the safe code for older versions.

Copy link
Member

Choose a reason for hiding this comment

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

It exists everywhere, you just need to add a dependency for the older builds. Let me try it out and tell you what to add. Stay tuned

Copy link
Member

Choose a reason for hiding this comment

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

Actually the project just had broken dependencies. I've fixed it up, please see: ericstj@0556f02

To test this locally you can run build -allConfigurations.

After doing so once you can run msbuild /t:BuildAllConfigurations on any project you want.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! it fixed reference issues, but now I have an issue with packaging:

D:\j\workspace\AllConfigurat---c1564d53\Tools\Packaging.targets(424,5): error MSB4018: The "HarvestPackage" task failed unexpectedly. [D:\j\workspace\AllConfigurat---c1564d53\src\System.Diagnostics.DiagnosticSource\pkg\System.Diagnostics.DiagnosticSource.pkgproj] 11:25:06 D:\j\workspace\AllConfigurat---c1564d53\Tools\Packaging.targets(424,5): error MSB4018: System.ArgumentException: An item with the same key has already been added. [D:\j\workspace\AllConfigurat---c1564d53\src\System.Diagnostics.DiagnosticSource\pkg\System.Diagnostics.DiagnosticSource.pkgproj]

Could you please help with it?

Copy link
Member

Choose a reason for hiding this comment

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

sure, let me have a look.

Copy link
Member

Choose a reason for hiding this comment

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

That's happening because this project was previously copying the netstandard1.3 build to the net46 folder. Now that you are building explicitly for netstandard1.3 you don't need to do that. See ericstj@85e6fab

Copy link
Member

Choose a reason for hiding this comment

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

I'll also fix up that error so that its a proper message: dotnet/buildtools#1388

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, it worked!

@@ -411,7 +412,9 @@ private string GenerateRootId()
return ret;
}

#if ALLOW_PARTIALLY_TRUSTED_CALLERS
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to do this

@vancem vancem merged commit 6b3ca00 into dotnet:master Mar 14, 2017
@morganbr
Copy link

Have you run secannotate on this? If not, you may have some broken cases.

lmolkova pushed a commit to lmolkova/corefx that referenced this pull request Mar 15, 2017
@lmolkova
Copy link
Author

@morganbr thanks for mentioning it. I found some errors related to usage of SecuritySafeCritical attribute on the targets which don't declare AllowPartiallyTrustedCallers, fix in #17132

@karelz karelz modified the milestone: 2.0.0 Mar 15, 2017
stephentoub added a commit that referenced this pull request Mar 18, 2017
…te_errors

Fix secannotate errors introduced by #17076 in DiagnosticSource
macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020
…lyTrustedCodeDiagSource

Enable AllowPartiallyTrustedCaller for DiagnosticSource on netfx

Commit migrated from dotnet/corefx@6b3ca00
macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020
macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020
…fix_secannotate_errors

Fix secannotate errors introduced by dotnet/corefx#17076 in DiagnosticSource

Commit migrated from dotnet/corefx@4ac8e65
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…lyTrustedCodeDiagSource

Enable AllowPartiallyTrustedCaller for DiagnosticSource on netfx

Commit migrated from dotnet/corefx@6b3ca00
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.

8 participants