Skip to content

Go: Remove global function step from local flow#21721

Merged
owen-mc merged 3 commits intogithub:mainfrom
owen-mc:go/remove-global-function-jump-step-from-local-flow
Apr 17, 2026
Merged

Go: Remove global function step from local flow#21721
owen-mc merged 3 commits intogithub:mainfrom
owen-mc:go/remove-global-function-jump-step-from-local-flow

Conversation

@owen-mc
Copy link
Copy Markdown
Contributor

@owen-mc owen-mc commented Apr 16, 2026

It would be good if the local flow relation was really local. There was one disjunct which isn't. I tried removing it and only one test failed, and on investigation this was easily fixed. I will run DCA and QA to see if there are any alert changes from this. My best guess is that it's somehow related to assigning global functions to variables (or using them as arguments), but since go doesn't use the lambda flow capabilities of the data flow library it probably isn't doing anything.

@github-actions github-actions bot added the Go label Apr 16, 2026
@owen-mc owen-mc force-pushed the go/remove-global-function-jump-step-from-local-flow branch from 06b9b76 to f275b9a Compare April 16, 2026 09:53
@owen-mc owen-mc force-pushed the go/remove-global-function-jump-step-from-local-flow branch from f275b9a to f6135b7 Compare April 16, 2026 10:15
@owen-mc
Copy link
Copy Markdown
Contributor Author

owen-mc commented Apr 17, 2026

I ran QA. It looks fine. There were a few alert changes in two repos, but investigating one of them locally I don't see any changes. The db seems to be slightly malformed - the locations don't work for many files. I don't know why this is - I didn't see any errors when building the db. There was one analysis that was much slower but it was for network reasons relating to downloading ubuntu archives (I believe canonical had some problems yesterday).

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Apr 17, 2026
@owen-mc owen-mc marked this pull request as ready for review April 17, 2026 10:08
@owen-mc owen-mc requested a review from a team as a code owner April 17, 2026 10:08
@owen-mc owen-mc requested review from Copilot and hvitved April 17, 2026 10:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR makes the Go “local flow” relation more strictly intra-procedural by removing a non-local disjunct involving global functions, while preserving the needed behavior for io/fs.WalkDir callback modeling.

Changes:

  • Remove the GlobalFunctionNode -> use case from basicLocalFlowStep to keep local flow truly local.
  • Update the io/fs.WalkDir additional taint step to handle both function literals and referenced global functions passed as the callback argument.
  • Update affected Go dataflow library-test .expected outputs accordingly.
Show a summary per file
File Description
go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll Removes the global-function-to-use disjunct from basicLocalFlowStep, tightening the definition of “local” flow.
go/ql/lib/semmle/go/frameworks/stdlib/IoFs.qll Refines WalkDir callback handling to account for function literals and global function references passed as arguments.
go/ql/test/library-tests/semmle/go/dataflow/PromotedFields/LocalFlowStep.expected Updates expected results to reflect removal of the global-function local-flow step.
go/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected Updates expected results to reflect removal of the global-function local-flow step.

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 0

This is no longer needed.
@owen-mc
Copy link
Copy Markdown
Contributor Author

owen-mc commented Apr 17, 2026

@hvitved You queried why this local flow step wasn't local here, and now that I look into it I see that it indeed isn't needed at all! I've also removed the exclusion that was needed from the "local flow is local" data flow consistency check.

Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion for getting rid of duplication.

Comment thread go/ql/lib/semmle/go/frameworks/stdlib/IoFs.qll Outdated
Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Assuming DCA is happy.

@owen-mc owen-mc merged commit 29b07d5 into github:main Apr 17, 2026
18 checks passed
@owen-mc owen-mc deleted the go/remove-global-function-jump-step-from-local-flow branch April 17, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Go no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants