Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix secannotate errors introduced by #17076 in DiagnosticSource#17132

Merged
stephentoub merged 2 commits intodotnet:masterfrom
lmolkova:dev/diag_source_fix_secannotate_errors
Mar 18, 2017
Merged

Fix secannotate errors introduced by #17076 in DiagnosticSource#17132
stephentoub merged 2 commits intodotnet:masterfrom
lmolkova:dev/diag_source_fix_secannotate_errors

Conversation

@lmolkova
Copy link

@lmolkova lmolkova commented Mar 15, 2017 •
edited by karelz
Loading

On the target that do not declare AllowPartiallyTrustedCallers (all except net46 and netfx), secannotate shows a critical error

      <type name="System.Diagnostics.Activity">
        <method name="GetRandomNumber()">
          <annotations>
            <critical>
              <rule name="TransparencyAnnotationsShouldNotConflict">
                <reason pass="1" sourceFile="c:\repo\corefx\src\System.Diagnostics.DiagnosticSource\src\System\Diagnostics\Activity.cs" sourceLine="415">'System.Diagnostics.Activity.GetRandomNumber()', a security critical member, is marked with a safe-critical annotation.  This annotation should be removed.</reason>
              </rule>
            </critical>
          </annotations>
        </method>
      </type>

This change uses SecuritySafeCritical attribute only on net46 and netfx.

@lmolkova lmolkova mentioned this pull request Mar 15, 2017
Merged
@stephentoub
Copy link
Member

stephentoub commented Mar 15, 2017

cc: @morganbr

@morganbr
Copy link

morganbr commented Mar 15, 2017

Looks good to me.

@stephentoub
Copy link
Member

stephentoub commented Mar 15, 2017

On the target that do not declare AllowPartiallyTrustedCallers (all except net46 and netfx)

@morganbr, do we care about the results of secannotate on non-net46/netfx? Does it even produce valid results in those cases, given the inconsistency with which these attributes (which are then nops) are applied on such platforms?

@morganbr
Copy link

morganbr commented Mar 17, 2017

@stephentoub , it's a bit helpful to ensure they're clean so other security tools don't flag them.

@stephentoub
Copy link
Member

stephentoub commented Mar 17, 2017

it's a bit helpful to ensure they're clean so other security tools don't flag them.

Ok. There are several hundred of these across the repo, so if that's an issue, we should delete / clean those up as well. https://github.com/dotnet/corefx/issues/12592

@stephentoub stephentoub merged commit 4ac8e65 into dotnet:master Mar 18, 2017
@lmolkova lmolkova mentioned this pull request Mar 22, 2017
Merged
@karelz karelz modified the milestone: 2.0.0 Mar 25, 2017
macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020
@stephentoub
Merge pull request dotnet/corefx#17132 from lmolkova/dev/diag_source_…
eeb9cb7 
…fix_secannotate_errors

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

Commit migrated from dotnet/corefx@4ac8e65
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@lmolkova lmolkova

@brianrob brianrob

@vancem vancem

Labels

area-System.Diagnostics.Tracing

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@lmolkova @stephentoub @morganbr @karelz @brianrob @vancem @dnfclas