vulncheck: remove synthetic nodes from call graph
This removes ~20% edges for k8s call graph. It also skips call stacks
that go through one level of indirection with wrappers.
The reason why this change is correct is as follows. Vulnerability db
entries never specify ssa wrappers as dbs are unaware of these. The only
type of wrappers applicable are the ones that are generated for calls to
pointer receivers where the source defines methods only on value
receivers. Vulnerability dbs do not care about this distinction nor do
the users, so it should be safe to inline the wrappers. This is exactly
what callgraph.DeleteSyntheticNodes does.
Change-Id: I2d76c0570d95f78ff4a2463ddf7cd95110fff15c
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/410054
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/vulncheck/source_test.go b/vulncheck/source_test.go
index ee42e52..4687ac9 100644
--- a/vulncheck/source_test.go
+++ b/vulncheck/source_test.go
@@ -568,3 +568,84 @@
}
}
}
+
+// TestNoSyntheticNodes checks that removing synthetic wrappers from
+// call graph still produces correct results.
+func TestNoSyntheticNodes(t *testing.T) {
+ e := packagestest.Export(t, packagestest.Modules, []packagestest.Module{
+ {
+ Name: "golang.org/entry",
+ Files: map[string]interface{}{
+ "x/x.go": `
+ package x
+
+ import "golang.org/amod/avuln"
+
+ type i interface {
+ Vuln1()
+ }
+
+ func X() {
+ v := &avuln.VulnData{}
+ var x i = v // to force creatation of wrapper method *avuln.VulnData.Vuln1
+ x.Vuln1()
+ }`,
+ },
+ },
+ {
+ Name: "golang.org/amod@v1.1.3",
+ Files: map[string]interface{}{"avuln/avuln.go": `
+ package avuln
+
+ type VulnData struct {}
+ func (v VulnData) Vuln1() {}
+ func (v VulnData) Vuln2() {}
+ `},
+ },
+ })
+ defer e.Cleanup()
+
+ // Load x as entry package.
+ pkgs, err := loadPackages(e, path.Join(e.Temp(), "entry/x"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(pkgs) != 1 {
+ t.Fatal("failed to load x test package")
+ }
+
+ cfg := &Config{
+ Client: testClient,
+ }
+ result, err := Source(context.Background(), Convert(pkgs), cfg)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if len(result.Vulns) != 2 {
+ t.Errorf("want 2 Vulns, got %d", len(result.Vulns))
+ }
+
+ var vuln *Vuln
+ for _, v := range result.Vulns {
+ if v.Symbol == "VulnData.Vuln1" && v.CallSink != 0 {
+ vuln = v
+ }
+ }
+
+ if vuln == nil {
+ t.Fatal("VulnData.Vuln1 should be deemed a called vulnerability")
+ }
+
+ stacks := CallStacks(result)[vuln]
+ if len(stacks) != 1 {
+ t.Fatalf("want 1 stack for VulnData.Vuln1; got %v stacks", len(stacks))
+ }
+
+ stack := stacks[0]
+ // We don't want the call stack X -> *VulnData.Vuln1 (wrapper) -> VulnData.Vuln1.
+ // We want X -> VulnData.Vuln1.
+ if len(stack) != 2 {
+ t.Errorf("want stack of length 2; got stack of length %v", len(stack))
+ }
+}
diff --git a/vulncheck/utils.go b/vulncheck/utils.go
index 0886a27..b8ded9d 100644
--- a/vulncheck/utils.go
+++ b/vulncheck/utils.go
@@ -74,7 +74,10 @@
fslice = forwardReachableFrom(entrySlice, vtaCg)
pruneSet(fslice, allFuncs)
- return vta.CallGraph(fslice, vtaCg)
+ cg := vta.CallGraph(fslice, vtaCg)
+ // TODO(#53206): can we do the same for intermediate call graphs?
+ cg.DeleteSyntheticNodes()
+ return cg
}
// siteCallees computes a set of callees for call site `call` given program `callgraph`.
