Dev/sraroseck/danmoseley/binskim suppress err997 ilonly

[Sasinkas]

No description provided.

[martin-reznik]

Recommend against merging this as written (Request changes).

I traced the reachability and this diff is a behavioral no-op for all 17 shipped rules — the 14 default-logging rules exclude IsILOnly in CanAnalyzePE, and the 3 managed-applicable rules (BA2004, BA2027, BA4001) already set LogPdbLoadException => false, so none reach the changed block. There are no SARIF baseline changes, consistent with that.

So the only real effect is on the SDK default that downstream/future rules inherit — and it flips the framework bias from fail-safe to fail-open:

In Analyze, the null-PDB early-return (crash protection) is welded to the ERR997 emission:

if (LogPdbLoadException) {
    if (target.Pdb == null && (!IsManaged || IsMixedMode || EnforcePdbLoadForManagedAssemblies)) {
        LogExceptionLoadingPdb(...);   // emit ERR997
        return;                         // protect the rule body from a null PDB
    }
}
this.AnalyzePortableExecutableAndPdb(context);   // runs with Pdb == null if we didn't return

Flipping EnforcePdbLoadForManagedAssemblies to false removes the early-return for managed-IL targets, so a managed-applicable rule that keeps the default LogPdbLoadException now runs with Pdb == null → either an NRE (no guard) or a silent skip / false negative (self-guarded). In a security scanner, a silent false negative is the worst outcome — "no result" reads as "clean." Note --ignorePdbLoadError already makes the loud path non-fatal, so the loudness this removes was already tunable.

The history reinforces this. #988 (fixing #986) handled a managed binary missing its PDB causing a fatal ExceptionLoadingPdb — caused by BA2027 (#657) forgetting LogPdbLoadException => false. The durable fix was the per-rule opt-out plus the RulePropertyTests allow-list. The fail-safe default is exactly what made that omission loud enough to catch and fix quickly; this change would make the same class of omission silent. BA2027 also shows "needs-PDB and managed-applicable" is a real, shipped shape (SourceLink data lives in the PDB), so the SDK default genuinely matters.

Also process-wise: empty description, no linked work item, no tests — and the change routes around the RulePropertyTests guard rather than extending it.

Suggestion: keep EnforcePdbLoadForManagedAssemblies => true. If a specific managed rule is too aggressive about PDBs, opt it out explicitly (the existing, test-enforced pattern). If there's a real downstream need behind this, could you share the scenario? The right fix is likely to decouple the welded concerns — always early-return on a null PDB (no NRE for any future rule) and let the flags control only whether it surfaces as ERR997 vs a non-fatal NotApplicable/informational — rather than flipping the whole default to silent-by-default.

[danmoseley]

Sounds fine to me. @Sasinkas LMK if you need me to do anything here but it would be great if you could just make the fix as I have moved teams and roles

[martin-reznik]

@danmoseley - can you remind me reasoning behind this change? From what I saw, it should be no-op for all current rules (it would not change behavior of any of them), but it would effectively flip the default - and this control should be ideally on the rule itself.

Maybe I am misunderstanding the impact?