Skip to content

Go: fix small issues highlighted by data flow consistency checks #20929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
owen-mc wants to merge 4 commits into
base: main
Choose a base branch
from
+19 −268
Open

Go: fix small issues highlighted by data flow consistency checks #20929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afb810c
Fix double space in change note
owen-mc Nov 27, 2025
9bf2070
Remove identity steps
owen-mc Nov 26, 2025
dcfa721
(Refactor) Make `lookThroughImplicitFieldRead` public
owen-mc Nov 26, 2025
a20c8cf
Add post-update nodes for implicit field read nodes
owen-mc Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/ql/lib/change-notes/2025-11-26-unexpected-frontend-errors-query-moved.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
---
category: breaking
---
* The query `go/unexpected-frontend-error` has been moved from the `codeql/go-queries` query to the `codeql-go-consistency-queries` query pack.
* The query `go/unexpected-frontend-error` has been moved from the `codeql/go-queries` query to the `codeql-go-consistency-queries` query pack.
5 changes: 5 additions & 0 deletions go/ql/lib/semmle/go/controlflow/IR.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1588,4 +1588,9 @@ module IR {
* in a field/method access, element access, or slice expression.
*/
EvalImplicitDerefInstruction implicitDerefInstruction(Expr e) { result = MkImplicitDeref(e) }

/** Gets the base of `insn`, if `insn` is an implicit field read. */
Instruction lookThroughImplicitFieldRead(Instruction insn) {
result = insn.(ImplicitFieldReadInstruction).getBaseInstruction()
}
}
6 changes: 5 additions & 1 deletion go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,11 @@ module Public {
exists(IR::MethodReadInstruction mri |
ce.getTarget() instanceof Method and
mri = IR::evalExprInstruction(ce.getCalleeExpr()) and
insn = mri.getReceiver()
// If a.x is reading a promoted field, and it's equivalent to a.b.c.x,
// then mri.getReceiver() will give us the implicit field read a.b.c
// and we want to have post-update nodes for a, the implicit field
// read a.b and the implicit field read a.b.c.
insn = IR::lookThroughImplicitFieldRead*(mri.getReceiver())
Copy link
Contributor

@hvitved hvitved Dec 5, 2025

Choose a reason for hiding this comment

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

Should this instead be a general rule saying: If we have a post-update node for a.b then we also need one for a?

Copy link
Contributor Author

@owen-mc owen-mc Dec 5, 2025

Choose a reason for hiding this comment

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

Looking more closely, I see that actually we already deal with reads of promoted fields (in insn = getAWrittenInsn()), and this change is actually about receivers of calls to promoted methods. (I've just updated the description.) To your point, doing that would make the predicate recursive and less transparent. Since I've determined exactly what was missing from the previous definition, I feel like the most straightforward thing is to just add it to the predicate.

Copy link
Contributor

@hvitved hvitved Dec 5, 2025

Choose a reason for hiding this comment

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

Ok, as long as the logic covers satisfies the implication I mention above.

)
) and
mutableType(insn.getResultType())
Expand Down
3 changes: 2 additions & 1 deletion go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) {
nodeFrom = instructionNode(pred) or
nodeFrom.(PostUpdateNode).getPreUpdateNode() = instructionNode(pred)
) and
nodeTo = instructionNode(succ)
nodeTo = instructionNode(succ) and
nodeTo != nodeFrom
)
or
// GlobalFunctionNode -> use
Expand Down
16 changes: 6 additions & 10 deletions go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,17 +384,13 @@ module SourceSinkInterpretationInput implements
}

private DataFlow::Node skipImplicitFieldReads(DataFlow::Node n) {
not exists(lookThroughImplicitFieldRead(n)) and result = n
not exists(IR::lookThroughImplicitFieldRead(n.asInstruction())) and result = n
or
result = skipImplicitFieldReads(lookThroughImplicitFieldRead(n))
}

private DataFlow::Node lookThroughImplicitFieldRead(DataFlow::Node n) {
result.asInstruction() =
n.(DataFlow::InstructionNode)
.asInstruction()
.(IR::ImplicitFieldReadInstruction)
.getBaseInstruction()
exists(DataFlow::Node mid |
mid.asInstruction() = IR::lookThroughImplicitFieldRead(n.asInstruction())
|
result = skipImplicitFieldReads(mid)
)
}

/** Provides additional sink specification logic. */
Expand Down
3 changes: 0 additions & 3 deletions go/ql/test/example-tests/snippets/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions go/ql/test/experimental/CWE-400/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions go/ql/test/experimental/CWE-522-DecompressionBombs/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,3 @@ reverseRead
| test.go:92:19:92:25 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| test.go:93:5:93:11 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| test.go:94:9:94:15 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
identityLocalStep
| test.go:114:37:114:38 | rc | Node steps to itself |
| test.go:198:27:198:29 | out | Node steps to itself |
| test.go:224:27:224:29 | out | Node steps to itself |
| test.go:249:27:249:29 | out | Node steps to itself |
| test.go:274:27:274:29 | out | Node steps to itself |
| test.go:299:27:299:29 | out | Node steps to itself |
| test.go:324:26:324:28 | out | Node steps to itself |
| test.go:349:26:349:28 | out | Node steps to itself |
| test.go:375:28:375:30 | out | Node steps to itself |
| test.go:403:28:403:30 | out | Node steps to itself |
| test.go:431:24:431:26 | out | Node steps to itself |
| test.go:463:26:463:28 | out | Node steps to itself |
| test.go:490:26:490:28 | out | Node steps to itself |
| test.go:517:27:517:29 | out | Node steps to itself |
| test.go:546:26:546:28 | out | Node steps to itself |
| test.go:571:26:571:28 | out | Node steps to itself |
| test.go:601:24:601:26 | out | Node steps to itself |
| test.go:614:15:614:21 | tarRead | Node steps to itself |
| test.go:622:3:622:7 | files | Node steps to itself |
| test.go:637:10:637:16 | tarRead | Node steps to itself |
| test.go:647:10:647:16 | tarRead | Node steps to itself |
3 changes: 0 additions & 3 deletions go/ql/test/experimental/CWE-942/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,3 @@ reverseRead
| CorsMisconfiguration.go:170:14:170:16 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| CorsMisconfiguration.go:194:17:194:19 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| CorsMisconfiguration.go:206:14:206:16 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
identityLocalStep
| CorsMisconfiguration.go:208:13:208:18 | origin | Node steps to itself |
| CorsMisconfiguration.go:235:6:235:8 | try | Node steps to itself |
6 changes: 0 additions & 6 deletions go/ql/test/library-tests/semmle/go/Types/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
reverseRead
| pkg1/embedding.go:56:29:56:39 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| pkg1/embedding.go:57:33:57:46 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| pkg1/embedding.go:58:14:58:22 | implicit read of field embedder | Origin of readStep is missing a PostUpdateNode. |
| pkg1/embedding.go:58:31:58:42 | implicit read of field embedder | Origin of readStep is missing a PostUpdateNode. |
| pkg1/tst.go:43:6:43:6 | t | Origin of readStep is missing a PostUpdateNode. |
| pkg1/tst.go:46:6:46:6 | t | Origin of readStep is missing a PostUpdateNode. |
| pkg1/tst.go:50:2:50:2 | t | Origin of readStep is missing a PostUpdateNode. |
| pkg1/tst.go:53:6:53:7 | t2 | Origin of readStep is missing a PostUpdateNode. |
| pkg1/tst.go:55:6:55:7 | t2 | Origin of readStep is missing a PostUpdateNode. |
| promoted.go:18:6:18:6 | t | Origin of readStep is missing a PostUpdateNode. |
37 changes: 0 additions & 37 deletions ...tests/semmle/go/dataflow/ExternalFlowInheritance/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions ...brary-tests/semmle/go/dataflow/ExternalTaintFlow/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,3 @@ reverseRead
| test.go:90:10:90:15 | taint8 | Origin of readStep is missing a PostUpdateNode. |
| test.go:104:12:104:18 | taint10 | Origin of readStep is missing a PostUpdateNode. |
| test.go:150:10:150:14 | slice | Origin of readStep is missing a PostUpdateNode. |
identityLocalStep
| test.go:92:3:92:3 | b | Node steps to itself |
| test.go:95:3:95:3 | b | Node steps to itself |
| test.go:106:3:106:3 | b | Node steps to itself |
| test.go:109:3:109:3 | b | Node steps to itself |
| test.go:119:3:119:3 | b | Node steps to itself |
| test.go:122:3:122:3 | b | Node steps to itself |
| test.go:125:3:125:3 | b | Node steps to itself |
| test.go:128:3:128:3 | b | Node steps to itself |
| test.go:138:3:138:3 | b | Node steps to itself |
| test.go:141:3:141:3 | b | Node steps to itself |
11 changes: 0 additions & 11 deletions ...brary-tests/semmle/go/dataflow/ExternalValueFlow/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,3 @@ reverseRead
| test.go:90:10:90:15 | taint8 | Origin of readStep is missing a PostUpdateNode. |
| test.go:104:12:104:18 | taint10 | Origin of readStep is missing a PostUpdateNode. |
| test.go:150:10:150:14 | slice | Origin of readStep is missing a PostUpdateNode. |
identityLocalStep
| test.go:92:3:92:3 | b | Node steps to itself |
| test.go:95:3:95:3 | b | Node steps to itself |
| test.go:106:3:106:3 | b | Node steps to itself |
| test.go:109:3:109:3 | b | Node steps to itself |
| test.go:119:3:119:3 | b | Node steps to itself |
| test.go:122:3:122:3 | b | Node steps to itself |
| test.go:125:3:125:3 | b | Node steps to itself |
| test.go:128:3:128:3 | b | Node steps to itself |
| test.go:138:3:138:3 | b | Node steps to itself |
| test.go:141:3:141:3 | b | Node steps to itself |
7 changes: 0 additions & 7 deletions ...library-tests/semmle/go/dataflow/PromotedMethods/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions ...ts/semmle/go/dataflow/flowsources/local/database/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions go/ql/test/library-tests/semmle/go/frameworks/Beego/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,3 @@ reverseRead
| test.go:318:20:318:22 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| test.go:324:17:324:19 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
| test.go:324:17:324:25 | implicit dereference | Origin of readStep is missing a PostUpdateNode. |
identityLocalStep
| test.go:278:3:278:14 | genericFiles | Node steps to itself |
| test.go:278:21:278:25 | files | Node steps to itself |
2 changes: 0 additions & 2 deletions .../test/library-tests/semmle/go/frameworks/Macaron/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

Loading