From 30cb7f98cd1aba45565123caf4cbd73772bb8b58 Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Wed, 6 Nov 2019 17:59:11 +0100 Subject: pkg/bisect: detect wrong bisections Detect bisection to merge commits and to commits that don't affect kernel binary (comments, other arches, whitespaces, etc). Such bisections are not reported in emails (but shown on web). Update #1271 --- pkg/bisect/bisect_test.go | 334 ++++++++++++++++++++++++---------------------- 1 file changed, 171 insertions(+), 163 deletions(-) (limited to 'pkg/bisect/bisect_test.go') diff --git a/pkg/bisect/bisect_test.go b/pkg/bisect/bisect_test.go index 72e12fbe3..04609e701 100644 --- a/pkg/bisect/bisect_test.go +++ b/pkg/bisect/bisect_test.go @@ -7,7 +7,6 @@ import ( "bytes" "fmt" "io/ioutil" - "math" "os" "strconv" "testing" @@ -21,13 +20,9 @@ import ( // testEnv will implement instance.BuilderTester. This allows us to // set bisect.env.inst to a testEnv object. type testEnv struct { - repo *vcs.TestRepo - r vcs.Repo - t *testing.T - fix bool - brokenStart float64 - brokenEnd float64 - culprit float64 + t *testing.T + r vcs.Repo + test BisectionTest } func (env *testEnv) BuildSyzkaller(repo, commit string) error { @@ -35,90 +30,68 @@ func (env *testEnv) BuildSyzkaller(repo, commit string) error { } func (env *testEnv) BuildKernel(compilerBin, userspaceDir, cmdlineFile, sysctlFile string, - kernelConfig []byte) (string, error) { - return "", nil -} - -func crashErrors(num int, title string) []error { - var errors []error - for i := 0; i < num; i++ { - errors = append(errors, &instance.CrashError{ - Report: &report.Report{ - Title: fmt.Sprintf("crashes at %v", title), - }, - }) + kernelConfig []byte) (string, string, error) { + commit := env.headCommit() + kernelSign := fmt.Sprintf("sign-%v", commit) + if commit >= env.test.sameBinaryStart && commit <= env.test.sameBinaryEnd { + kernelSign = "same-sign" } - return errors + return "", kernelSign, nil } -func nilErrors(num int) []error { - var errors []error - for i := 0; i < num; i++ { - errors = append(errors, nil) +func (env *testEnv) Test(numVMs int, reproSyz, reproOpts, reproC []byte) ([]error, error) { + commit := env.headCommit() + if commit >= env.test.brokenStart && commit <= env.test.brokenEnd { + return nil, fmt.Errorf("broken build") } - return errors + if !env.test.fix && commit >= env.test.culprit || env.test.fix && commit < env.test.culprit { + return crashErrors(numVMs, "crash occurs"), nil + } + return make([]error, numVMs), nil } -func (env *testEnv) Test(numVMs int, reproSyz, reproOpts, reproC []byte) ([]error, error) { - hc, err := env.r.HeadCommit() +func (env *testEnv) headCommit() int { + com, err := env.r.HeadCommit() if err != nil { env.t.Fatal(err) } - commit, err := strconv.ParseFloat(hc.Title, 64) + commit, err := strconv.ParseUint(com.Title, 10, 64) if err != nil { - env.t.Fatalf("invalid commit title: %v", hc.Title) - } - var e error - var res []error - if commit >= env.brokenStart && commit <= env.brokenEnd { - e = fmt.Errorf("broken build") - } else if commit < env.culprit && !env.fix || commit >= env.culprit && env.fix { - res = nilErrors(numVMs) - } else { - res = crashErrors(numVMs, "crash occurs") + env.t.Fatalf("invalid commit title: %v", com.Title) } - return res, e + return int(commit) } -type Ctx struct { - t *testing.T - baseDir string - repo *vcs.TestRepo - r vcs.Repo - cfg *Config - inst *testEnv - originRepo *vcs.TestRepo -} - -func NewCtx(t *testing.T, fix bool, brokenStart, brokenEnd, culprit float64, commit string) *Ctx { - baseDir, err := ioutil.TempDir("", "syz-git-test") +func runBisection(t *testing.T, test BisectionTest) (*Result, error) { + baseDir, err := ioutil.TempDir("", "syz-bisect-test") if err != nil { t.Fatal(err) } - originRepo := vcs.CreateTestRepo(t, baseDir, "originRepo") + defer os.RemoveAll(baseDir) + repo := vcs.CreateTestRepo(t, baseDir, "repo") + if !repo.SupportsBisection() { + t.Skip("bisection is unsupported by git (probably too old version)") + } for rv := 4; rv < 10; rv++ { for i := 0; i < 6; i++ { - originRepo.CommitChange(fmt.Sprintf("%v", rv*100+i)) + repo.CommitChange(fmt.Sprintf("%v", rv*100+i)) if i == 0 { - originRepo.SetTag(fmt.Sprintf("v%v.0", rv)) + repo.SetTag(fmt.Sprintf("v%v.0", rv)) } } } - if !originRepo.SupportsBisection() { - t.Skip("bisection is unsupported by git (probably too old version)") - } - repo := vcs.CloneTestRepo(t, baseDir, "repo", originRepo) r, err := vcs.NewRepo("test", "64", repo.Dir) if err != nil { t.Fatal(err) } - sc, err := r.GetCommitByTitle(commit) + sc, err := r.GetCommitByTitle(fmt.Sprint(test.startCommit)) if err != nil { t.Fatal(err) } + trace := new(bytes.Buffer) cfg := &Config{ - Fix: fix, - Trace: new(bytes.Buffer), + Fix: test.fix, + Trace: trace, Manager: mgrconfig.Config{ TargetOS: "test", TargetVMArch: "64", @@ -126,177 +99,212 @@ func NewCtx(t *testing.T, fix bool, brokenStart, brokenEnd, culprit float64, com KernelSrc: repo.Dir, }, Kernel: KernelConfig{ - Repo: originRepo.Dir, + Repo: repo.Dir, Commit: sc.Hash, }, } inst := &testEnv{ - repo: repo, - r: r, - t: t, - fix: fix, - brokenStart: brokenStart, - brokenEnd: brokenEnd, - culprit: culprit, - } - c := &Ctx{ - t: t, - baseDir: baseDir, - repo: repo, - r: r, - cfg: cfg, - inst: inst, - originRepo: originRepo, + t: t, + r: r, + test: test, } - return c + res, err := runImpl(cfg, r, r.(vcs.Bisecter), inst) + t.Log(trace.String()) + return res, err } -type BisectionTests struct { +type BisectionTest struct { // input environment name string fix bool - startCommit string - brokenStart float64 - brokenEnd float64 + startCommit int + brokenStart int + brokenEnd int + // Range of commits that result in the same kernel binary signature. + sameBinaryStart int + sameBinaryEnd int // expected output - errIsNil bool + expectErr bool + expectRep bool + noopChange bool commitLen int - repIsNil bool - oldestLatest string + oldestLatest int // input and output - culprit float64 + culprit int } func TestBisectionResults(t *testing.T) { t.Parallel() - var tests = []BisectionTests{ + tests := []BisectionTest{ // Tests that bisection returns the correct cause commit. { - name: "bisect cause finds cause", - fix: false, - startCommit: "905", - brokenStart: math.Inf(0), - brokenEnd: 0, - errIsNil: true, + name: "cause-finds-cause", + startCommit: 905, commitLen: 1, - repIsNil: false, + expectRep: true, culprit: 602, }, // Tests that cause bisection returns error when crash does not reproduce // on the original commit. { - name: "bisect cause does not repro", - fix: false, - startCommit: "400", - brokenStart: math.Inf(0), - brokenEnd: 0, - errIsNil: false, - commitLen: 0, - repIsNil: true, - culprit: math.Inf(0), + name: "cause-does-not-repro", + startCommit: 400, + expectErr: true, }, // Tests that no commits are returned when crash occurs on oldest commit // for cause bisection. { - name: "bisect cause crashes oldest", - fix: false, - startCommit: "905", - brokenStart: math.Inf(0), - brokenEnd: 0, - errIsNil: true, + name: "cause-crashes-oldest", + startCommit: 905, commitLen: 0, - repIsNil: false, + expectRep: true, culprit: 0, - oldestLatest: "400", + oldestLatest: 400, }, - // Tests that more than 1 commit is returned when cause bisection is - // inconclusive. + // Tests that more than 1 commit is returned when cause bisection is inconclusive. { - name: "bisect cause inconclusive", - fix: false, - startCommit: "802", + name: "cause-inconclusive", + startCommit: 802, brokenStart: 500, brokenEnd: 700, - errIsNil: true, commitLen: 14, - repIsNil: true, culprit: 605, }, // Tests that bisection returns the correct fix commit. { - name: "bisect fix finds fix", + name: "fix-finds-fix", fix: true, - startCommit: "400", - brokenStart: math.Inf(0), - brokenEnd: 0, - errIsNil: true, + startCommit: 400, commitLen: 1, - repIsNil: true, culprit: 500, }, // Tests that fix bisection returns error when crash does not reproduce // on the original commit. { - name: "bisect fix does not repro", + name: "fix-does-not-repro", fix: true, - startCommit: "905", - brokenStart: math.Inf(0), - brokenEnd: 0, - errIsNil: false, - commitLen: 0, - repIsNil: true, - culprit: 0, + startCommit: 905, + expectErr: true, }, // Tests that no commits are returned when crash occurs on HEAD // for fix bisection. { - name: "bisect fix crashes HEAD", + name: "fix-crashes-HEAD", fix: true, - startCommit: "400", - brokenStart: math.Inf(0), - brokenEnd: 0, - errIsNil: true, + startCommit: 400, commitLen: 0, - repIsNil: false, + expectRep: true, culprit: 1000, - oldestLatest: "905", + oldestLatest: 905, }, - // Tests that more than 1 commit is returned when fix bisection is - // inconclusive. + // Tests that more than 1 commit is returned when fix bisection is inconclusive. { - name: "bisect fix inconclusive", + name: "fix-inconclusive", fix: true, - startCommit: "400", + startCommit: 400, brokenStart: 500, brokenEnd: 600, - errIsNil: true, commitLen: 8, - repIsNil: true, culprit: 501, }, + { + name: "cause-same-binary", + startCommit: 905, + commitLen: 1, + expectRep: true, + culprit: 503, + sameBinaryStart: 502, + sameBinaryEnd: 503, + noopChange: true, + }, + { + name: "cause-same-binary-off-by-one", + startCommit: 905, + commitLen: 1, + expectRep: true, + culprit: 503, + sameBinaryStart: 400, + sameBinaryEnd: 502, + }, + { + name: "cause-same-binary-off-by-one-2", + startCommit: 905, + commitLen: 1, + expectRep: true, + culprit: 503, + sameBinaryStart: 503, + sameBinaryEnd: 905, + }, + { + name: "fix-same-binary", + fix: true, + startCommit: 400, + commitLen: 1, + culprit: 503, + sameBinaryStart: 502, + sameBinaryEnd: 504, + noopChange: true, + }, } for _, test := range tests { - t.Run(fmt.Sprintf("%v", test.name), func(t *testing.T) { - c := NewCtx(t, test.fix, test.brokenStart, test.brokenEnd, test.culprit, test.startCommit) - defer os.RemoveAll(c.baseDir) - commits, rep, com, err := runImpl(c.cfg, c.r, c.r.(vcs.Bisecter), c.inst) - if test.errIsNil && err != nil || !test.errIsNil && err == nil { - t.Fatalf("returned error: '%v'", err) + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + if test.expectErr && + (test.commitLen != 0 || + test.expectRep || + test.oldestLatest != 0 || + test.culprit != 0) { + t.Fatalf("expecting non-default values on error") + } + if test.brokenStart > test.brokenEnd { + t.Fatalf("bad broken start/end: %v/%v", + test.brokenStart, test.brokenEnd) + } + if test.sameBinaryStart > test.sameBinaryEnd { + t.Fatalf("bad same binary start/end: %v/%v", + test.sameBinaryStart, test.sameBinaryEnd) + } + res, err := runBisection(t, test) + if test.expectErr != (err != nil) { + t.Fatalf("returned error: %v", err) + } + if err != nil { + if res != nil { + t.Fatalf("got both result and error: '%v' %+v", err, *res) + } + return + } + if len(res.Commits) != test.commitLen { + t.Fatalf("expected %d commits got %d commits", test.commitLen, len(res.Commits)) } - if len(commits) != test.commitLen { - t.Fatalf("expected %d commits got %d commits", test.commitLen, len(commits)) + expectedTitle := fmt.Sprint(test.culprit) + if len(res.Commits) == 1 && expectedTitle != res.Commits[0].Title { + t.Fatalf("expected commit '%v' got '%v'", expectedTitle, res.Commits[0].Title) } - expectedTitle := fmt.Sprintf("%v", test.culprit) - if len(commits) == 1 && expectedTitle != commits[0].Title { - t.Fatalf("expected commit '%v' got '%v'", expectedTitle, commits[0].Title) + if test.expectRep != (res.Report != nil) { + t.Fatalf("got rep: %v, want: %v", res.Report, test.expectRep) } - if test.repIsNil && rep != nil || !test.repIsNil && rep == nil { - t.Fatalf("returned rep: '%v'", err) + if res.NoopChange != test.noopChange { + t.Fatalf("got noop change: %v, want: %v", res.NoopChange, test.noopChange) } - if test.oldestLatest != "" && test.oldestLatest != com.Title || - test.oldestLatest == "" && com != nil { - t.Fatalf("expected latest/oldest: '%v' got '%v'", test.oldestLatest, com.Title) + if test.oldestLatest != 0 && fmt.Sprint(test.oldestLatest) != res.Commit.Title || + test.oldestLatest == 0 && res.Commit != nil { + t.Fatalf("expected latest/oldest: %v got '%v'", + test.oldestLatest, res.Commit.Title) } }) } } + +func crashErrors(num int, title string) []error { + var errors []error + for i := 0; i < num; i++ { + errors = append(errors, &instance.CrashError{ + Report: &report.Report{ + Title: fmt.Sprintf("crashes at %v", title), + }, + }) + } + return errors +} -- cgit mrf-deployment