Go: fix models for built-in functions

[owen-mc]

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

• Go: Fix getType on ImplicitVarArgsSlice
• Go: fix hasQualifiedName and models-as-data for built-in functions

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]

⚠️ 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]

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]

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

[owen-mc]

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]

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

[owen-mc]

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.

[owen-mc]

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]

Lost results due to min and max being variadic functions

[owen-mc]

This has been fixed now.

[smowton]

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

[owen-mc]

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.