Go: Use shared guards library by owen-mc · Pull Request #22365 · github/codeql

owen-mc · 2026-08-17T16:31:01Z

Instantiate the shared guards library for Go, port over all uses of the Go-specific ConditionGuardNode and deprecate it.

Go-specific issues encountered:

functions with multiple return values.
that the syntactic receiver and the semantic receiver of a method call may be different, e.g. in case of a promoted method or an implicit dereference.
functions with named result parameters may implicitly return values, rather than explicitly as part of a return statement.

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

owen-mc · 2026-10-06T16:28:09Z

"Run QL for QL" is failing for all PRs at the moment. I don't think it is related to this PR.

Copilot

Copilot review overview

🟡 Changes recommended

The deprecation change note incorrectly states that the legacy class has no instances.

Review effort: Balanced
Findings: 1

Open (1)

Correct inaccurate claim that deprecated API has no instances · New

What changed in this PR

Migrates Go guard reasoning to the shared control-flow guards library and updates dependent analyses, models, and tests.

Changes:

Adds the Go Guard/GuardValue implementation and shared-library extension points.
Migrates guard consumers and barrier models to the new APIs.
Deprecates the legacy guard API and expands regression coverage.

File	Description
`shared/controlflow/codeql/controlflow/Guards.qll`	Extends shared guard capabilities.
`java/ql/lib/semmle/code/java/controlflow/Guards.qll`	Adapts Java to the extended interface.
`go/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/main.go`	Adds switch-based flag coverage.
`go/ql/test/query-tests/Security/CWE-209/test.go`	Adds switch-flow coverage.
`go/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected`	Updates expected findings.
`go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go`	Expands wrapper-guard tests.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ql`	Adds guard API test queries.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/guards.go`	Adds comprehensive guard fixtures.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ext.yml`	Adds barrier-guard models.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected`	Records expected guard results.
`go/ql/test/experimental/CWE-942/CorsMisconfiguration.go`	Adds switch-based CORS coverage.
`go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected`	Updates expected CORS findings.
`go/ql/test/experimental/CWE-942/CONSISTENCY/DataFlowConsistency.expected`	Updates consistency expectations.
`go/ql/src/Security/CWE-327/InsecureTLS.ql`	Uses shared flag-control reasoning.
`go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql`	Uses shared flag-control reasoning.
`go/ql/src/Security/CWE-209/StackTraceExposure.ql`	Migrates debug-flag barriers.
`go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql`	Migrates regexp guard reasoning.
`go/ql/src/InconsistentCode/LengthComparisonOffByOne.ql`	Migrates comparison guards.
`go/ql/src/InconsistentCode/ConstantLengthComparison.ql`	Migrates length guards.
`go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll`	Uses shared range guards.
`go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql`	Migrates CORS guard logic.
`go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql`	Migrates sensitive-condition guards.
`go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll`	Adds flag-control abstraction.
`go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll`	Migrates barrier-guard infrastructure.
`go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll`	Migrates unreachable-region guards.
`go/ql/lib/semmle/go/dataflow/ExternalFlow.qll`	Supports all modeled guard values.
`go/ql/lib/semmle/go/controlflow/IR.qll`	Clarifies conditional IR handling.
`go/ql/lib/semmle/go/controlflow/Guards.qll`	Instantiates shared guards for Go.
`go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll`	Deprecates legacy guard APIs.
`go/ql/lib/change-notes/2026-08-17-shared-guards.md`	Announces the shared guard API.
`go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md`	Announces legacy API deprecation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

owen-mc · 2026-10-10T17:08:00Z

There seems to be a performance problem which is causing some DCA jobs to time out. I'll investigate on Tuesday.

Add the complete guard regression suite and fold the shared hooks and Go-specific correctness fixes into the initial adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Move simple data-flow and security consumers to shared guards, including value-aware barriers and the corrected feature-flag semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

owen-mc · 2026-10-14T13:05:12Z

@aschackmull This is now ready to review.

I've resolved the big performance issues. The latest DCA run now shows and average 5% increase in analysis time. I investigated this in a few cases and wasn't able to easily reduce it - I think it is acceptable because we are now doing a more thorough analysis that we used to, e.g. conditions being propagated through variables and phi nodes.

The test failures seem spurious.

aschackmull · 2026-10-19T07:44:23Z

+
+  class Expr extends G::Expr {
+    /** Gets the associated control flow node. */
+    CfgImpl::Cfg::ControlFlowNode getControlFlowNode() { result = IR::evalExprInstruction(this) }


This is intended to be result.injects(this). AFAICS the only difference is for defers, is that a relevant distinction or could we use injects here?

aschackmull · 2026-10-19T07:51:24Z

+   * This supports implicit equality tests that do not have a corresponding source expression,
+   * such as comparisons between a switch expression and a case expression.


This should already work without an additional input predicate.

The existing Case abstraction only handles a single constant case expression, assuming the switch expression is the value being constrained. Go has two additional cases:

The constant may be the switch expression, for example switch false { case x: ... }. Here we need to infer x == false, but asConstantCase() cannot expose x as the compared expression.

A clause may contain multiple expressions, for example case 1, 2. Taking its non-match path implies both value != 1 and value != 2, while taking its match path does not identify which equality succeeded.

I suggest replacing asConstantCase() with an outcome-sensitive relation such as:

predicate constantComparison( boolean caseOutcome, Expr comparedExpr, ConstantExpr constant, boolean equal );

This would model constants on either side and the asymmetric implications of multi-expression cases in the shared library, avoiding the lower-level equalityBranchEdge input.

aschackmull · 2026-10-19T08:06:15Z

+    }
+  }
+
+  predicate caseOutcomeBlock(Case guard, CfgImpl::Cfg::BasicBlock outcomeBlock, boolean branch) {


What's the purpose of this signature extension? Could you elaborate on what didn't work without it? It seems quite narrow as it's restricted to default cases, so I wonder what hole it's trying to plug.

You're right, this can be removed. It was trying to preserve the old Go situation where we had a fact for "default matched", but that isn't actually needed. I'll push a commit to remove it.

aschackmull · 2026-10-19T08:12:25Z

+  /**
+   * Holds if `outcomeBlock` contains a control flow node indicating that
+   * `guard` evaluated to `branch`.
+   */
+  default predicate booleanOutcomeBlock(Expr guard, BasicBlock outcomeBlock, boolean branch) {
+    none()
+  }


I'm aware that the Guards library likely needs an extension for covering Guards with after-value nodes that are fully determined by prior branches. I.e. a disjunction in a test position like if (a || b) is currently not working as a guard - only the individual a and b are guards here. Is it correctly understood that it is this particular corner-case that you're trying to handle here? I'd much prefer if such an extension was the subject of its own PR instead. Really, this ties into a potentially larger Guards refactor to better take into account the structure of the new CFG.

That's right. I'll remove it from this PR.

aschackmull · 2026-10-19T08:17:35Z

+     * Holds if `def` defines the implicit return value represented by `method`, or is an SSA
+     * definition traversed while determining that value.
+     */
+    default predicate relevantImplicitReturnSsaDefinition(
+      NonOverridableMethod method, SsaDefinition def
+    ) {
+      any()


Whoah! The default is a huge cartesian product?

Oops, yes, that default is too broad. It was intended as an unrestrictive fallback, with evaluation seeded by implicitReturnDefinition, but it is still conceptually a Cartesian product and is unsafe if used unbound. I’ll change the default to none() and require languages supporting implicit returns to provide a bounded implementation.

aschackmull · 2026-10-19T08:26:22Z

+private module LogicInput implements GuardsImpl::LogicInputSig {
+  final private class FinalSsaDefinition = GoSsa::SsaDefinition;
+
+  class SsaDefinition extends FinalSsaDefinition {


Note: If Go used the MakeSsa module exposed by the shared SSA lib to build its SSA classes then the result should be directly pluggable with a one-line import here. (Just something to consider for potential followup).

aschackmull · 2026-10-19T08:31:22Z

+     * This supports languages where a return statement can return a named result
+     * without containing a return expression.
+     */
+    default predicate implicitReturnDefinition(NonOverridableMethod method, SsaDefinition def) {


This seems very Go-specific. I wonder whether we could instead just include any expressions being written to def as the return expressions instead? That way we'd stay within the existing signature and not need this extension.

The issue with that is that result parameters are implicitly initialized to a zero value, without a corresponding expression, just a synthetic SSA definition. See for example the test guardMultiNamed. (This is probably quite a common pattern in real Go code, to have an Error result parameter, implicitly initialized to nil, and only change its value on branches where there is an error.)

Other possible approaches:

use SSA definitions/writes as return-value sources;

introduce a shared ReturnValue abstraction that can wrap either an expression or a synthetic SSA value.

For approach 1, I note that another input predicate I've added, additionalSsaDefinitionValue, could be part of the solution, e.g. if we generalize implicitReturnDefinition to something like:

/** * Holds if `def` is the SSA value returned as the result represented by `method`. */ default predicate returnValueDefinition( NonOverridableMethod method, SsaDefinition def ) { none() }

Then the shared library can trace possible values back and additionalSsaDefinitionValue can be used for implicit initializations.

aschackmull · 2026-10-19T08:49:56Z

+  predicate additionalSsaDefinitionValue(SsaDefinition def, GuardValue value) {
+    exists(IR::EvalImplicitInitInstruction init |
+      def.(GoSsa::SsaExplicitDefinition).getInstruction() = init and
+      value.asBooleanValue() = init.getBoolValue()
+    )
+  }


The addition here looks reasonable, but perhaps somewhat narrow. Was this found to make a difference? Is there a test exercising this case?

The relevant test is conditionalNamedResultIsNotNil:

func conditionalNamedResultIsNotNil(pointer *int) (valid bool) { if pointer == nil { return } valid = true return }

At the merge point, valid is a phi over the explicit true and the implicit false.

aschackmull · 2026-10-19T09:05:34Z

+       * Holds if `def` evaluating to `defVal` implies that `guard` evaluates to `guardVal`.
+       */
+      predicate ssaImpliesGuard(
+        NonOverridableMethod method, SsaDefinition def, GuardValue defVal, Guard guard,
+        GuardValue guardVal


Could you walk me through what this addition is covering?

This covers naked returns of named Go results, where there is no return expression to seed the existing analysis. It walks backwards from the SSA definition read in the synthetic return epilogue, through any phis, to recover the guard outcome implied by the returned value. For example, if the result is a phi of implicit false on p == nil and explicit true otherwise, observing true implies p != nil.

Align Go with Java and C# by deriving useful default-branch facts from the preceding non-match edges rather than treating the default clause itself as a true guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Keep support for compound guards represented by CFG after-value nodes out of the Go shared-guards migration. This behavior can be reconsidered as part of a broader Guards/CFG integration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Use an empty default for relevantImplicitReturnSsaDefinition instead of the Cartesian product. Languages supporting implicit returns must provide a bounded implementation; Go already does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions Bot added documentation Go labels Aug 17, 2026

github-advanced-security Bot found potential problems Aug 17, 2026

View reviewed changes

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

Comment thread go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll Fixed

owen-mc force-pushed the go/shared-guards branch from f8092fc to 57db4c8 Compare August 17, 2026 19:16

github-advanced-security Bot found potential problems Aug 17, 2026

View reviewed changes

owen-mc force-pushed the go/shared-guards branch 2 times, most recently from def5d61 to 888eae3 Compare August 18, 2026 11:45

owen-mc force-pushed the go/shared-guards branch from 888eae3 to 82e42b4 Compare September 20, 2026 22:01

github-advanced-security Bot found potential problems Sep 20, 2026

View reviewed changes

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

owen-mc mentioned this pull request Sep 22, 2026

Go: un-deprecate ParenExpr #22650

Merged

owen-mc force-pushed the go/shared-guards branch from c60e983 to 45fd28e Compare September 22, 2026 15:40

owen-mc force-pushed the go/shared-guards branch from 17bf057 to e3e1dcb Compare September 30, 2026 22:46

github-advanced-security Bot found potential problems Sep 30, 2026

View reviewed changes

owen-mc force-pushed the go/shared-guards branch from e3e1dcb to 05e44c8 Compare October 1, 2026 13:53

github-actions Bot added the Java label Oct 1, 2026

owen-mc force-pushed the go/shared-guards branch from 05e44c8 to bf97136 Compare October 6, 2026 14:46

github-advanced-security Bot found potential problems Oct 6, 2026

View reviewed changes

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

owen-mc force-pushed the go/shared-guards branch from bf97136 to 97e898b Compare October 6, 2026 15:42

owen-mc mentioned this pull request Oct 6, 2026

QL-for-QL: fix parsing of overlay[local?] and overlay[caller?] annotations #22763

Merged

owen-mc marked this pull request as ready for review October 6, 2026 16:28

Copilot AI balanced review requested due to automatic review settings October 6, 2026 16:28

owen-mc requested review from a team as code owners October 6, 2026 16:28

Copilot started reviewing on behalf of owen-mc October 6, 2026 16:28

View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md Outdated

owen-mc requested a review from aschackmull October 9, 2026 15:51

owen-mc force-pushed the go/shared-guards branch from fc8d706 to c05aecb Compare October 9, 2026 15:52

owen-mc force-pushed the go/shared-guards branch 3 times, most recently from c4929fe to dfff37f Compare October 13, 2026 19:20

owen-mc and others added 7 commits October 13, 2026 23:45

Go: instantiate the shared guards library

0c0ebf9

Add the complete guard regression suite and fold the shared hooks and Go-specific correctness fixes into the initial adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Go: migrate simple guard consumers

8007acf

Move simple data-flow and security consumers to shared guards, including value-aware barriers and the corrected feature-flag semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Go: migrate remaining guard consumers

41c9afd

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Go: deprecate ConditionGuardNode

d5a6f9e

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Document Go shared guard migration

9ca7e14

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Allow functions with multiple return values in Guards logic

be72021

Refactor: add Expr.getControlFlowNode and Expr.getBasicBlock

32d4595

owen-mc force-pushed the go/shared-guards branch from dfff37f to 32d4595 Compare October 14, 2026 08:37

aschackmull reviewed Oct 19, 2026

View reviewed changes

Use injects

480e631

aschackmull reviewed Oct 19, 2026

View reviewed changes

owen-mc and others added 3 commits October 19, 2026 11:07

Guards: remove default-case outcome hook

6aedc51

Align Go with Java and C# by deriving useful default-branch facts from the preceding non-match edges rather than treating the default clause itself as a true guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

		* This supports implicit equality tests that do not have a corresponding source expression,
		* such as comparisons between a switch expression and a case expression.

Conversation

owen-mc commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

owen-mc commented Oct 10, 2026

Uh oh!

owen-mc commented Oct 14, 2026 • 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!

owen-mc Oct 19, 2026 • 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!

owen-mc Oct 19, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

owen-mc commented Aug 17, 2026 •

edited

Loading

owen-mc commented Oct 6, 2026 •

edited

Loading

owen-mc commented Oct 14, 2026 •

edited

Loading

owen-mc Oct 19, 2026 •

edited

Loading

owen-mc Oct 19, 2026 •

edited

Loading