Go: fix models for built-in functions #16413

owen-mc · 2024-05-03T10:55:55Z

Our model for append was wrong, as append is variadic. We were missing a model for copy. Here are the package docs and the relevant part of the spec.

I had to fix two bugs which I made into separate PRs

And I fixed one bug in this PR which is hard to otherwise test for.

I also span out #16444 to make the value flow tests have the same results as the taint flow tests.

github-actions · 2024-05-03T10:58:57Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

go

Generated file changes for go

Changes to framework-coverage-go.rst:

-    `Standard library <https://pkg.go.dev/std>`_,"````, ``archive/*``, ``bufio``, ``bytes``, ``cmp``, ``compress/*``, ``container/*``, ``context``, ``crypto``, ``crypto/*``, ``database/*``, ``debug/*``, ``embed``, ``encoding``, ``encoding/*``, ``errors``, ``expvar``, ``flag``, ``fmt``, ``go/*``, ``hash``, ``hash/*``, ``html``, ``html/*``, ``image``, ``image/*``, ``index/*``, ``io``, ``io/*``, ``log``, ``log/*``, ``maps``, ``math``, ``math/*``, ``mime``, ``mime/*``, ``net``, ``net/*``, ``os``, ``os/*``, ``path``, ``path/*``, ``plugin``, ``reflect``, ``reflect/*``, ``regexp``, ``regexp/*``, ``slices``, ``sort``, ``strconv``, ``strings``, ``sync``, ``sync/*``, ``syscall``, ``syscall/*``, ``testing``, ``testing/*``, ``text/*``, ``time``, ``time/*``, ``unicode``, ``unicode/*``, ``unsafe``",8,578,
+    `Standard library <https://pkg.go.dev/std>`_,"````, ``archive/*``, ``bufio``, ``bytes``, ``cmp``, ``compress/*``, ``container/*``, ``context``, ``crypto``, ``crypto/*``, ``database/*``, ``debug/*``, ``embed``, ``encoding``, ``encoding/*``, ``errors``, ``expvar``, ``flag``, ``fmt``, ``go/*``, ``hash``, ``hash/*``, ``html``, ``html/*``, ``image``, ``image/*``, ``index/*``, ``io``, ``io/*``, ``log``, ``log/*``, ``maps``, ``math``, ``math/*``, ``mime``, ``mime/*``, ``net``, ``net/*``, ``os``, ``os/*``, ``path``, ``path/*``, ``plugin``, ``reflect``, ``reflect/*``, ``regexp``, ``regexp/*``, ``slices``, ``sort``, ``strconv``, ``strings``, ``sync``, ``sync/*``, ``syscall``, ``syscall/*``, ``testing``, ``testing/*``, ``text/*``, ``time``, ``time/*``, ``unicode``, ``unicode/*``, ``unsafe``",8,581,
-    Totals,,20,871,24
+    Totals,,20,874,24

Changes to framework-coverage-go.csv:

- ,,,2,,,,,2
+ ,,,5,,,,,5

smowton

Haven't reviewed the test changes, but the code changes look good to me.

smowton · 2024-05-07T17:26:05Z

go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

@@ -455,8 +455,8 @@ module Public {
    CallNode getCallNode() { result = call }

    override Type getType() {
-      exists(Function f | f = call.getTarget() |
-        result = f.getParameterType(f.getNumParameter() - 1)
+      exists(SignatureType t | t = call.getCall().getCalleeType() |


Suggested change

exists(SignatureType t | t = call.getCall().getCalleeType() |

// Note this differs from `call.getTarget()`'s parameter types for polymorphic builtins like `append`.

exists(SignatureType t | t = call.getCall().getCalleeType() |

smowton · 2024-05-07T17:27:39Z

To check: are there old-school QL models relating to either function that are now duplicative or redundant?

owen-mc · 2024-05-11T06:54:50Z

I've now updated this to remove the old-style models and make sure all tests changes are ones we expect/want. It's ready to review again.

smowton · 2024-05-13T13:04:58Z

go/ql/lib/ext/builtin.model.yml

+      - ["", "", False, "append", "", "", "Argument[1].ArrayElement", "ReturnValue.ArrayElement", "value", "manual"]
+      - ["", "", False, "copy", "", "", "Argument[1].ArrayElement", "Argument[0].ArrayElement", "value", "manual"]
+      - ["", "", False, "max", "", "", "Argument[0]", "ReturnValue", "value", "manual"]


Per https://pkg.go.dev/builtin#max min and max are variadic

Oops, I hadn't realised that there could be more than two arguments. Although, despite what those docs say, if I try to implement the model like that then it doesn't work. It seems that the compiler has special-cased these functions, and they just have an invalid signature and aren't considered variadic. Maybe because of lack of generics originally stopping them from being implemented normally. I can get the type for the CallExpr and for three arguments it's func(int, int, int) int. It's a bit annoying, as we don't have a way of saying "any argument" in models-as-data (according to our docs, at least). I guess we could do 0..1000. I'll experiment.

I asked on the Gophers slack and someone pointed me to this comment. It turns out that min and max were only added in Go 1.21, and after some debate they decided to make them built-in functions because it's nicer to write max(a, b) than Math.Max(a, b) and they seem to provide a fundamental utility (though Go survived without it for a long time).

smowton · 2024-05-13T13:05:27Z

go/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected

@@ -127,12 +127,10 @@
 | main.go:64:7:64:18 | call to min | main.go:64:2:64:2 | definition of a |
 | main.go:64:11:64:11 | x | main.go:64:7:64:18 | call to min |
 | main.go:64:14:64:14 | y | main.go:64:7:64:18 | call to min |
-| main.go:64:17:64:17 | z | main.go:64:7:64:18 | call to min |


Lost results due to min and max being variadic functions

This has been fixed now.

smowton · 2024-05-13T13:06:22Z

go/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalTaintStep.expected

@@ -3,13 +3,6 @@
 | main.go:38:13:38:13 | 1 | main.go:38:7:38:20 | slice literal |
 | main.go:38:16:38:16 | 2 | main.go:38:7:38:20 | slice literal |
 | main.go:38:19:38:19 | 3 | main.go:38:7:38:20 | slice literal |
-| main.go:39:15:39:15 | s | main.go:39:8:39:25 | call to append |


You're sure taint still works as expected here, just via different steps?

This test, and the value flow counterpart, only show steps without any reads or stores. The model for append is now a value step with a store, so it isn't shown. The test in go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/test.go still shows taint flow through append.

Although the documentation makes them look variadic (and generic), they are actually special-cased in the compiler. Like all built-in functions they don't have a signature type, but the type of `min(a, b, c)` is `func(int, int, int) int` and not `func(int, ...int) int`. Go doesn't allow open-ended ranges for argument indices in models-as-data specifications (though Ruby and Python do), so I've used `1..1000`.

github-actions bot added the Go label May 3, 2024

github-actions bot added the documentation label May 3, 2024

owen-mc force-pushed the go/fix-builtin-models branch 3 times, most recently from 0a0d54d to 95eeefd Compare May 7, 2024 15:05

owen-mc marked this pull request as ready for review May 7, 2024 15:06

owen-mc requested a review from a team as a code owner May 7, 2024 15:06

smowton previously approved these changes May 7, 2024

View reviewed changes

owen-mc dismissed smowton’s stale review via 79cdec8 May 7, 2024 20:20

This was referenced May 8, 2024

Go: Fix getType on ImplicitVarArgsSlice #16457

Merged

Go: fix hasQualifiedName and models-as-data for built-in functions #16458

Merged

owen-mc marked this pull request as draft May 8, 2024 14:48

owen-mc mentioned this pull request May 8, 2024

Go: Use value flow instead of taint flow for go/incorrect-integer-conversion #16305

Merged

owen-mc force-pushed the go/fix-builtin-models branch from 79cdec8 to 694c896 Compare May 8, 2024 20:14

owen-mc mentioned this pull request May 9, 2024

Go: Fix test expectations #16464

Merged

owen-mc force-pushed the go/fix-builtin-models branch from 694c896 to 6100995 Compare May 9, 2024 15:47

owen-mc added 12 commits May 10, 2024 23:40

Fix model for append - it is variadic

e857eed

Add model for built-in function copy

1322ce3

Add models for min and max built-in functions

fcdac0a

Remove old-style models of built-in functions

f95ccd2

Rename test folder ExternalFlow to ExternalTaintFlow

84b7319

Create copy of ExternalTaintFlow called ExternalValueFlow

c816412

Update ExternalValueFlow to use value flow

eab1569

Add tests for copy (taint passes, value fails)

86e2b84

Local flow steps for flow summaries for built-in functions

d5aa08d

Add change note

8cc9838

Fix elementWrite to include flow summaries

e1c85e3

Accept MaD renumbering for edge provenance

7694618

owen-mc added 2 commits May 10, 2024 23:40

Accept expected test changes

f8e17fe

Two tests have different paths but same results

f7e6bf7

owen-mc force-pushed the go/fix-builtin-models branch from 6100995 to f7e6bf7 Compare May 10, 2024 22:42

owen-mc marked this pull request as ready for review May 11, 2024 06:49

smowton reviewed May 13, 2024

View reviewed changes

owen-mc force-pushed the go/fix-builtin-models branch from dc4cbf2 to 31e3422 Compare May 14, 2024 13:19

owen-mc added 2 commits May 14, 2024 14:40

Test changes due to model renumbering

827d15a

owen-mc force-pushed the go/fix-builtin-models branch from 31e3422 to 827d15a Compare May 14, 2024 13:41

smowton approved these changes May 14, 2024

View reviewed changes

owen-mc merged commit 145873f into github:main May 14, 2024
14 checks passed

owen-mc deleted the go/fix-builtin-models branch May 14, 2024 20:03

owen-mc mentioned this pull request May 30, 2024

Go: Add back taint models for append and copy #16624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: fix models for built-in functions #16413

Go: fix models for built-in functions #16413

owen-mc commented May 3, 2024 •

edited

Loading

github-actions bot commented May 3, 2024 •

edited

Loading

go

Generated file changes for go

smowton left a comment

smowton May 7, 2024

smowton commented May 7, 2024

owen-mc commented May 11, 2024

smowton May 13, 2024

owen-mc May 14, 2024 •

edited

Loading

owen-mc May 14, 2024

smowton May 13, 2024

owen-mc May 14, 2024

smowton May 13, 2024

owen-mc May 14, 2024

	exists(SignatureType t \| t = call.getCall().getCalleeType() \|
	// Note this differs from `call.getTarget()`'s parameter types for polymorphic builtins like `append`.
	exists(SignatureType t \| t = call.getCall().getCalleeType() \|

Go: fix models for built-in functions #16413

Go: fix models for built-in functions #16413

Conversation

owen-mc commented May 3, 2024 • edited Loading

github-actions bot commented May 3, 2024 • edited Loading

go

Generated file changes for go

smowton left a comment

Choose a reason for hiding this comment

smowton May 7, 2024

Choose a reason for hiding this comment

smowton commented May 7, 2024

owen-mc commented May 11, 2024

smowton May 13, 2024

Choose a reason for hiding this comment

owen-mc May 14, 2024 • edited Loading

Choose a reason for hiding this comment

owen-mc May 14, 2024

Choose a reason for hiding this comment

smowton May 13, 2024

Choose a reason for hiding this comment

owen-mc May 14, 2024

Choose a reason for hiding this comment

smowton May 13, 2024

Choose a reason for hiding this comment

owen-mc May 14, 2024

Choose a reason for hiding this comment

owen-mc commented May 3, 2024 •

edited

Loading

github-actions bot commented May 3, 2024 •

edited

Loading

owen-mc May 14, 2024 •

edited

Loading