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 --- dashboard/app/bisect_test.go | 109 +++++++++++++ dashboard/app/entities.go | 19 +++ dashboard/app/jobs.go | 83 +++++----- dashboard/app/reporting.go | 4 +- dashboard/dashapi/dashapi.go | 8 + pkg/bisect/bisect.go | 187 +++++++++++++--------- pkg/bisect/bisect_test.go | 334 ++++++++++++++++++++------------------- pkg/instance/instance.go | 17 +- pkg/vcs/git.go | 8 +- pkg/vcs/git_repo_test.go | 5 + pkg/vcs/vcs.go | 1 + syz-ci/jobs.go | 30 ++-- tools/syz-bisect/bisect.go | 2 +- tools/syz-testbuild/testbuild.go | 4 +- 14 files changed, 506 insertions(+), 305 deletions(-) diff --git a/dashboard/app/bisect_test.go b/dashboard/app/bisect_test.go index 0512ab9fc..e4362ae8a 100644 --- a/dashboard/app/bisect_test.go +++ b/dashboard/app/bisect_test.go @@ -90,6 +90,7 @@ func TestBisectCause(t *testing.T) { Error: []byte("bisect error 3"), } c.expectOK(c.client2.JobDone(done)) + c.expectNoEmail() // BisectCause #2 pollResp = c.client2.pollJobs(build.Manager) @@ -542,6 +543,114 @@ https://goo.gl/tpsmEJ#testing-patches`, } } +func TestBisectWrong(t *testing.T) { + // Test bisection results with BisectResultMerge/BisectResultNoop flags set. + // If any of these set, the result must not be reported separately, + // as part of bug report during upstreamming, nor should affect CC list. + c := NewCtx(t) + defer c.Close() + + build := testBuild(1) + c.client2.UploadBuild(build) + for i := 0; i < 4; i++ { + var flags dashapi.JobDoneFlags + switch i { + case 0: + case 1: + flags = dashapi.BisectResultMerge + case 2: + flags = dashapi.BisectResultNoop + case 3: + flags = dashapi.BisectResultMerge | dashapi.BisectResultNoop + default: + t.Fatalf("assign flags") + } + t.Logf("iteration %v: flags=%v", i, flags) + + crash := testCrashWithRepro(build, i) + c.client2.ReportCrash(crash) + c.client2.pollEmailBug() + + { + pollResp := c.client2.pollJobs(build.Manager) + done := &dashapi.JobDoneReq{ + ID: pollResp.ID, + Flags: flags, + Build: *build, + Log: []byte("bisect log"), + Commits: []dashapi.Commit{ + { + Hash: "111111111111111111111111", + Title: "kernel: break build", + Author: "hacker@kernel.org", + AuthorName: "Hacker Kernelov", + Date: time.Date(2000, 2, 9, 4, 5, 6, 7, time.UTC), + }, + }, + } + done.Build.ID = pollResp.ID + c.expectOK(c.client2.JobDone(done)) + if i == 0 { + msg := c.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, fmt.Sprintf("syzbot has bisected this bug to:"))) + } else { + c.expectNoEmail() + } + } + { + c.advanceTime(31 * 24 * time.Hour) + pollResp := c.client2.pollJobs(build.Manager) + done := &dashapi.JobDoneReq{ + ID: pollResp.ID, + Flags: flags, + Build: *build, + Log: []byte("bisectfix log 4"), + CrashTitle: "bisectfix crash title 4", + CrashLog: []byte("bisectfix crash log 4"), + CrashReport: []byte("bisectfix crash report 4"), + Commits: []dashapi.Commit{ + { + Hash: "46e65cb4a0448942ec316b24d60446bbd5cc7827", + Title: "kernel: add a fix", + Author: "fixer@kernel.org", + AuthorName: "Author Kernelov", + Date: time.Date(2000, 2, 9, 4, 5, 6, 7, time.UTC), + }, + }, + } + done.Build.ID = pollResp.ID + c.expectOK(c.client2.JobDone(done)) + if i == 0 { + msg := c.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, "syzbot suspects this bug was fixed by commit:")) + } + } + { + // Auto-upstreamming. + c.advanceTime(31 * 24 * time.Hour) + msg := c.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, "Sending this report upstream")) + msg = c.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, "syzbot found the following crash on:")) + if i == 0 { + c.expectTrue(strings.Contains(msg.Body, "The bug was bisected to:")) + c.expectEQ(msg.To, []string{ + "bugs@syzkaller.com", + "default@maintainers.com", + "hacker@kernel.org", + }) + } else { + c.expectTrue(!strings.Contains(msg.Body, "The bug was bisected to:")) + c.expectEQ(msg.To, []string{ + "bugs@syzkaller.com", + "default@maintainers.com", + }) + } + } + c.expectNoEmail() + } +} + func TestBisectCauseAncient(t *testing.T) { c := NewCtx(t) defer c.Close() diff --git a/dashboard/app/entities.go b/dashboard/app/entities.go index 0d368a953..26e422d06 100644 --- a/dashboard/app/entities.go +++ b/dashboard/app/entities.go @@ -183,6 +183,7 @@ type Job struct { BuildID string Log int64 // reference to Log text entity Error int64 // reference to Error text entity, if set job failed + Flags JobFlags Reported bool // have we reported result back to user? } @@ -195,6 +196,24 @@ const ( JobBisectFix ) +type JobFlags int64 + +const ( + // Parallel to dashapi.JobDoneFlags, see comments there. + BisectResultMerge JobFlags = 1 << iota + BisectResultNoop +) + +func (job *Job) isUnreliableBisect() bool { + if job.Type != JobBisectCause && job.Type != JobBisectFix { + panic(fmt.Sprintf("bad job type %v", job.Type)) + } + // If a bisection points to a merge or a commit that does not affect the kernel binary, + // it is considered an unreliable/wrong result and should not be reported in emails. + return job.Flags&BisectResultMerge != 0 || + job.Flags&BisectResultNoop != 0 +} + // Text holds text blobs (crash logs, reports, reproducers, etc). type Text struct { Namespace string diff --git a/dashboard/app/jobs.go b/dashboard/app/jobs.go index e7c908f2d..7b9fbf22b 100644 --- a/dashboard/app/jobs.go +++ b/dashboard/app/jobs.go @@ -507,45 +507,16 @@ func doneJob(c context.Context, req *dashapi.JobDoneReq) error { Date: com.Date, }) } + job.BuildID = req.Build.ID + job.CrashTitle = req.CrashTitle + job.Finished = now + job.Flags = JobFlags(req.Flags) if job.Type == JobBisectCause || job.Type == JobBisectFix { // Update bug.BisectCause/Fix status and also remember current bug reporting to send results. - bug := new(Bug) - bugKey := jobKey.Parent() - if err := db.Get(c, bugKey, bug); err != nil { - return fmt.Errorf("job %v: failed to get bug: %v", jobID, err) - } - result := BisectYes - if len(req.Error) != 0 { - result = BisectError - } - if job.Type == JobBisectCause { - bug.BisectCause = result - } else { - bug.BisectFix = result - } - // If the crash still occurs on HEAD, update the bug's LastTime so that it will be - // retried after 30 days. - crashesOnHead(c, bug, job, req, now) - if _, err := db.Put(c, bugKey, bug); err != nil { - return fmt.Errorf("failed to put bug: %v", err) - } - _, bugReporting, _, _, _ := currentReporting(c, bug) - if bugReporting == nil || bugReporting.Reported.IsZero() { - // The bug is either already closed or not yet reported in the current reporting, - // either way we don't need to report it. If it wasn't reported, it will be reported - // with the bisection results. - job.Reported = true - } else { - job.Reporting = bugReporting.Name + if err := updateBugBisection(c, job, jobKey, req, now); err != nil { + return err } } - if job.Error != 0 && job.Type != JobTestPatch { - // Don't report errors for non-user-initiated jobs. - job.Reported = true - } - job.BuildID = req.Build.ID - job.CrashTitle = req.CrashTitle - job.Finished = now if _, err := db.Put(c, jobKey, job); err != nil { return fmt.Errorf("failed to put job: %v", err) } @@ -555,12 +526,44 @@ func doneJob(c context.Context, req *dashapi.JobDoneReq) error { return db.RunInTransaction(c, tx, &db.TransactionOptions{XG: true, Attempts: 30}) } -func crashesOnHead(c context.Context, bug *Bug, job *Job, req *dashapi.JobDoneReq, now time.Time) { - if job.Type != JobBisectFix || req.Error != nil || len(req.Commits) != 0 || len(req.CrashLog) == 0 { - return +func updateBugBisection(c context.Context, job *Job, jobKey *db.Key, req *dashapi.JobDoneReq, now time.Time) error { + bug := new(Bug) + bugKey := jobKey.Parent() + if err := db.Get(c, bugKey, bug); err != nil { + return fmt.Errorf("job %v: failed to get bug: %v", req.ID, err) + } + result := BisectYes + if len(req.Error) != 0 { + result = BisectError + } + if job.Type == JobBisectCause { + bug.BisectCause = result + } else { + bug.BisectFix = result + } + // If the crash still occurs on HEAD, update the bug's LastTime so that it will be + // retried after 30 days. + if job.Type == JobBisectFix && req.Error == nil && len(req.Commits) == 0 && len(req.CrashLog) != 0 { + bug.BisectFix = BisectNot + bug.LastTime = now + } + if _, err := db.Put(c, bugKey, bug); err != nil { + return fmt.Errorf("failed to put bug: %v", err) + } + _, bugReporting, _, _, _ := currentReporting(c, bug) + // The bug is either already closed or not yet reported in the current reporting, + // either way we don't need to report it. If it wasn't reported, it will be reported + // with the bisection results. + if bugReporting == nil || bugReporting.Reported.IsZero() || + // Don't report errors for non-user-initiated jobs. + job.Error != 0 || + // Don't report unreliable/wrong bisections. + job.isUnreliableBisect() { + job.Reported = true + } else { + job.Reporting = bugReporting.Name } - bug.BisectFix = BisectNot - bug.LastTime = now + return nil } func pollCompletedJobs(c context.Context, typ string) ([]*dashapi.BugReport, error) { diff --git a/dashboard/app/reporting.go b/dashboard/app/reporting.go index cb3f52cdd..c0a76106e 100644 --- a/dashboard/app/reporting.go +++ b/dashboard/app/reporting.go @@ -339,7 +339,7 @@ func createBugReport(c context.Context, bug *Bug, crash *Crash, crashKey *db.Key return nil, err } job = job1 - if crash1.ReproC != 0 || crash.ReproC == 0 { + if !job.isUnreliableBisect() && (crash1.ReproC != 0 || crash.ReproC == 0) { // Don't override the crash in this case, // otherwise we will always think that we haven't reported the C repro. crash, crashKey = crash1, crashKey1 @@ -408,7 +408,7 @@ func createBugReport(c context.Context, bug *Bug, crash *Crash, crashKey *db.Key if bugReporting.CC != "" { rep.CC = strings.Split(bugReporting.CC, "|") } - if bug.BisectCause == BisectYes { + if bug.BisectCause == BisectYes && !job.isUnreliableBisect() { rep.BisectCause = bisectFromJob(c, rep, job) } if err := fillBugReport(c, rep, bug, bugReporting, build); err != nil { diff --git a/dashboard/dashapi/dashapi.go b/dashboard/dashapi/dashapi.go index 4b2060463..b05f3fbf5 100644 --- a/dashboard/dashapi/dashapi.go +++ b/dashboard/dashapi/dashapi.go @@ -158,6 +158,7 @@ type JobDoneReq struct { // If there is 1 commits: bisection result (cause or fix). // If there are more than 1: suspected commits due to skips (broken build/boot). Commits []Commit + Flags JobDoneFlags } type JobType int @@ -168,6 +169,13 @@ const ( JobBisectFix ) +type JobDoneFlags int64 + +const ( + BisectResultMerge JobDoneFlags = 1 << iota // bisected to a merge commit + BisectResultNoop // commit does not affect resulting kernel binary +) + func (dash *Dashboard) JobPoll(req *JobPollReq) (*JobPollResp, error) { resp := new(JobPollResp) err := dash.Query("job_poll", req, resp) diff --git a/pkg/bisect/bisect.go b/pkg/bisect/bisect.go index 1458e141e..5a6197c32 100644 --- a/pkg/bisect/bisect.go +++ b/pkg/bisect/bisect.go @@ -56,7 +56,7 @@ type env struct { bisecter vcs.Bisecter commit *vcs.Commit head *vcs.Commit - inst instance.BuilderTester + inst instance.Env numTests int buildTime time.Duration testTime time.Duration @@ -64,44 +64,53 @@ type env struct { const NumTests = 10 // number of tests we do per commit -// Run does the bisection and returns: -// - if bisection is conclusive, the single cause/fix commit +// Result describes bisection result: +// - if bisection is conclusive, the single cause/fix commit in Commits // - for cause bisection report is the crash on the cause commit // - for fix bisection report is nil -// - *vcs.Commit will be nil -// - if bisection is inconclusive, range of potential cause/fix commits +// - Commit is nil +// - NoopChange is set if the commit did not cause any change in the kernel binary +// (bisection result it most likely wrong) +// - if bisection is inconclusive, range of potential cause/fix commits in Commits // - report is nil in such case -// - *vcs.Commit will be nil -// - if the crash still happens on the oldest release/HEAD (for cause/fix bisection correspondingly), -// no commits and the crash report on the oldest release/HEAD; and *vcs.Commit will point to -// the oldest/latest commit where crash happens. -// - if the crash is not reproduced on the start commit, an error, *vcs.Commit will be nil -func Run(cfg *Config) ([]*vcs.Commit, *report.Report, *vcs.Commit, error) { +// - Commit is nil +// - if the crash still happens on the oldest release/HEAD (for cause/fix bisection correspondingly) +// - no commits in Commits +// - the crash report on the oldest release/HEAD; +// - Commit points to the oldest/latest commit where crash happens. +type Result struct { + Commits []*vcs.Commit + Report *report.Report + Commit *vcs.Commit + NoopChange bool +} + +// Run does the bisection and returns either the Result, +// or, if the crash is not reproduced on the start commit, an error. +func Run(cfg *Config) (*Result, error) { if err := checkConfig(cfg); err != nil { - return nil, nil, nil, err + return nil, err } cfg.Manager.Cover = false // it's not supported somewhere back in time repo, err := vcs.NewRepo(cfg.Manager.TargetOS, cfg.Manager.Type, cfg.Manager.KernelSrc) if err != nil { - return nil, nil, nil, err + return nil, err } bisecter, ok := repo.(vcs.Bisecter) if !ok { - return nil, nil, nil, fmt.Errorf("bisection is not implemented for %v", cfg.Manager.TargetOS) + return nil, fmt.Errorf("bisection is not implemented for %v", cfg.Manager.TargetOS) } inst, err := instance.NewEnv(&cfg.Manager) if err != nil { - return nil, nil, nil, err + return nil, err } if _, err = repo.CheckoutBranch(cfg.Kernel.Repo, cfg.Kernel.Branch); err != nil { - return nil, nil, nil, err + return nil, err } - return runImpl(cfg, repo, bisecter, inst) } -func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.BuilderTester) ( - []*vcs.Commit, *report.Report, *vcs.Commit, error) { +func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.Env) (*Result, error) { env := &env{ cfg: cfg, repo: repo, @@ -110,7 +119,7 @@ func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.Bu } head, err := repo.HeadCommit() if err != nil { - return nil, nil, nil, err + return nil, err } env.head = head if cfg.Fix { @@ -119,91 +128,106 @@ func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.Bu env.log("bisecting cause commit starting from %v", cfg.Kernel.Commit) } start := time.Now() - commits, rep, bad, err := env.bisect() + res, err := env.bisect() env.log("revisions tested: %v, total time: %v (build: %v, test: %v)", env.numTests, time.Since(start), env.buildTime, env.testTime) if err != nil { env.log("error: %v", err) - return nil, nil, nil, err + return nil, err } - if len(commits) == 0 { + if len(res.Commits) == 0 { if cfg.Fix { env.log("the crash still happens on HEAD") } else { env.log("the crash already happened on the oldest tested release") } - env.log("commit msg: %v", bad.Title) - env.log("crash: %v\n%s", rep.Title, rep.Report) - return nil, rep, bad, nil + env.log("commit msg: %v", res.Commit.Title) + env.log("crash: %v\n%s", res.Report.Title, res.Report.Report) + return res, nil } what := "bad" if cfg.Fix { what = "good" } - if len(commits) > 1 { + if len(res.Commits) > 1 { env.log("bisection is inconclusive, the first %v commit could be any of:", what) - for _, com := range commits { + for _, com := range res.Commits { env.log("%v", com.Hash) } - return commits, nil, nil, nil + return res, nil } - com := commits[0] + com := res.Commits[0] env.log("first %v commit: %v %v", what, com.Hash, com.Title) env.log("cc: %q", com.CC) - if rep != nil { - env.log("crash: %v\n%s", rep.Title, rep.Report) + if res.Report != nil { + env.log("crash: %v\n%s", res.Report.Title, res.Report.Report) } - return commits, rep, nil, nil + return res, nil } -func (env *env) bisect() ([]*vcs.Commit, *report.Report, *vcs.Commit, error) { +func (env *env) bisect() (*Result, error) { cfg := env.cfg var err error if err := build.Clean(cfg.Manager.TargetOS, cfg.Manager.TargetVMArch, cfg.Manager.Type, cfg.Manager.KernelSrc); err != nil { - return nil, nil, nil, fmt.Errorf("kernel clean failed: %v", err) + return nil, fmt.Errorf("kernel clean failed: %v", err) } env.log("building syzkaller on %v", cfg.Syzkaller.Commit) if err := env.inst.BuildSyzkaller(cfg.Syzkaller.Repo, cfg.Syzkaller.Commit); err != nil { - return nil, nil, nil, err + return nil, err } com, err := env.repo.CheckoutCommit(cfg.Kernel.Repo, cfg.Kernel.Commit) if err != nil { - return nil, nil, nil, err + return nil, err } env.commit = com - res, _, rep0, err := env.test() + testRes, err := env.test() if err != nil { - return nil, nil, nil, err - } else if res != vcs.BisectBad { - return nil, nil, nil, fmt.Errorf("the crash wasn't reproduced on the original commit") + return nil, err + } else if testRes.verdict != vcs.BisectBad { + return nil, fmt.Errorf("the crash wasn't reproduced on the original commit") } bad, good, rep1, err := env.commitRange() if err != nil { - return nil, nil, nil, err + return nil, err } if rep1 != nil { - return nil, rep1, bad, nil // still not fixed/happens on the oldest release + return &Result{Report: rep1, Commit: bad}, nil // still not fixed/happens on the oldest release } - reports := make(map[string]*report.Report) - reports[cfg.Kernel.Commit] = rep0 + results := map[string]*testResult{cfg.Kernel.Commit: testRes} commits, err := env.bisecter.Bisect(bad.Hash, good.Hash, cfg.Trace, func() (vcs.BisectResult, error) { - res, com, rep, err := env.test() - reports[com.Hash] = rep + testRes1, err := env.test() + if err != nil { + return 0, err + } + results[testRes1.com.Hash] = testRes1 if cfg.Fix { - if res == vcs.BisectBad { - res = vcs.BisectGood - } else if res == vcs.BisectGood { - res = vcs.BisectBad + if testRes1.verdict == vcs.BisectBad { + testRes1.verdict = vcs.BisectGood + } else if testRes1.verdict == vcs.BisectGood { + testRes1.verdict = vcs.BisectBad } } - return res, err + return testRes1.verdict, err }) - var rep *report.Report + if err != nil { + return nil, err + } + res := &Result{ + Commits: commits, + } if len(commits) == 1 { - rep = reports[commits[0].Hash] + com := commits[0] + if testRes := results[com.Hash]; testRes != nil { + res.Report = testRes.rep + if testRes.kernelSign != "" && len(com.Parents) == 1 { + if prevRes := results[com.Parents[0]]; prevRes != nil { + res.NoopChange = testRes.kernelSign == prevRes.kernelSign + } + } + } } - return commits, rep, nil, err + return res, nil } func (env *env) commitRange() (*vcs.Commit, *vcs.Commit, *report.Report, error) { @@ -218,12 +242,12 @@ func (env *env) commitRangeForFix() (*vcs.Commit, *vcs.Commit, *report.Report, e if _, err := env.repo.SwitchCommit(env.head.Hash); err != nil { return nil, nil, nil, err } - res, _, rep, err := env.test() + res, err := env.test() if err != nil { return nil, nil, nil, err } - if res != vcs.BisectGood { - return env.head, nil, rep, nil + if res.verdict != vcs.BisectGood { + return env.head, nil, res.rep, nil } return env.head, env.commit, nil, nil } @@ -245,45 +269,57 @@ func (env *env) commitRangeForBug() (*vcs.Commit, *vcs.Commit, *report.Report, e if err != nil { return nil, nil, nil, err } - res, _, rep, err := env.test() + res, err := env.test() if err != nil { return nil, nil, nil, err } - if res == vcs.BisectGood { + if res.verdict == vcs.BisectGood { return lastBad, com, nil, nil } - if res == vcs.BisectBad { + if res.verdict == vcs.BisectBad { lastBad = com - lastRep = rep + lastRep = res.rep } } return lastBad, nil, lastRep, nil } -func (env *env) test() (vcs.BisectResult, *vcs.Commit, *report.Report, error) { +type testResult struct { + verdict vcs.BisectResult + com *vcs.Commit + rep *report.Report + kernelSign string +} + +func (env *env) test() (*testResult, error) { cfg := env.cfg env.numTests++ current, err := env.repo.HeadCommit() if err != nil { - return 0, nil, nil, err + return nil, err } bisectEnv, err := env.bisecter.EnvForCommit(cfg.BinDir, current.Hash, cfg.Kernel.Config) if err != nil { - return 0, nil, nil, err + return nil, err } compilerID, err := build.CompilerIdentity(bisectEnv.Compiler) if err != nil { - return 0, nil, nil, err + return nil, err } env.log("testing commit %v with %v", current.Hash, compilerID) buildStart := time.Now() if err := build.Clean(cfg.Manager.TargetOS, cfg.Manager.TargetVMArch, cfg.Manager.Type, cfg.Manager.KernelSrc); err != nil { - return 0, nil, nil, fmt.Errorf("kernel clean failed: %v", err) + return nil, fmt.Errorf("kernel clean failed: %v", err) } - _, err = env.inst.BuildKernel(bisectEnv.Compiler, cfg.Kernel.Userspace, + _, kernelSign, err := env.inst.BuildKernel(bisectEnv.Compiler, cfg.Kernel.Userspace, cfg.Kernel.Cmdline, cfg.Kernel.Sysctl, bisectEnv.KernelConfig) env.buildTime += time.Since(buildStart) + res := &testResult{ + verdict: vcs.BisectSkip, + com: current, + kernelSign: kernelSign, + } if err != nil { if verr, ok := err.(*osutil.VerboseError); ok { env.log("%v", verr.Title) @@ -294,27 +330,28 @@ func (env *env) test() (vcs.BisectResult, *vcs.Commit, *report.Report, error) { } else { env.log("%v", err) } - return vcs.BisectSkip, current, nil, nil + return res, nil } testStart := time.Now() results, err := env.inst.Test(NumTests, cfg.Repro.Syz, cfg.Repro.Opts, cfg.Repro.C) env.testTime += time.Since(testStart) if err != nil { env.log("failed: %v", err) - return vcs.BisectSkip, current, nil, nil + return res, nil } bad, good, rep := env.processResults(current, results) - res := vcs.BisectSkip + res.rep = rep + res.verdict = vcs.BisectSkip if bad != 0 { - res = vcs.BisectBad + res.verdict = vcs.BisectBad } else if NumTests-good-bad > NumTests/3*2 { // More than 2/3 of instances failed with infrastructure error, // can't reliably tell that the commit is good. - res = vcs.BisectSkip + res.verdict = vcs.BisectSkip } else if good != 0 { - res = vcs.BisectGood + res.verdict = vcs.BisectGood } - return res, current, rep, nil + return res, nil } func (env *env) processResults(current *vcs.Commit, results []error) (bad, good int, rep *report.Report) { 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 +} diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index e5370d7ee..ccb5265d1 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -27,9 +27,9 @@ import ( "github.com/google/syzkaller/vm" ) -type BuilderTester interface { +type Env interface { BuildSyzkaller(string, string) error - BuildKernel(string, string, string, string, []byte) (string, error) + BuildKernel(string, string, string, string, []byte) (string, string, error) Test(numVMs int, reproSyz, reproOpts, reproC []byte) ([]error, error) } @@ -37,7 +37,7 @@ type env struct { cfg *mgrconfig.Config } -func NewEnv(cfg *mgrconfig.Config) (BuilderTester, error) { +func NewEnv(cfg *mgrconfig.Config) (Env, error) { if !vm.AllowsOvercommit(cfg.Type) { return nil, fmt.Errorf("test instances are not supported for %v VMs", cfg.Type) } @@ -89,7 +89,7 @@ func (env *env) BuildSyzkaller(repo, commit string) error { } func (env *env) BuildKernel(compilerBin, userspaceDir, cmdlineFile, sysctlFile string, kernelConfig []byte) ( - string, error) { + string, string, error) { imageDir := filepath.Join(env.cfg.Workdir, "image") params := &build.Params{ TargetOS: env.cfg.TargetOS, @@ -103,17 +103,18 @@ func (env *env) BuildKernel(compilerBin, userspaceDir, cmdlineFile, sysctlFile s SysctlFile: sysctlFile, Config: kernelConfig, } - if _, err := build.Image(params); err != nil { - return "", err + kernelSign, err := build.Image(params) + if err != nil { + return "", "", err } if err := SetConfigImage(env.cfg, imageDir, true); err != nil { - return "", err + return "", "", err } kernelConfigFile := filepath.Join(imageDir, "kernel.config") if !osutil.IsExist(kernelConfigFile) { kernelConfigFile = "" } - return kernelConfigFile, nil + return kernelConfigFile, kernelSign, nil } func SetConfigImage(cfg *mgrconfig.Config, imageDir string, reliable bool) error { diff --git a/pkg/vcs/git.go b/pkg/vcs/git.go index 359c3d68e..5a6de4b3f 100644 --- a/pkg/vcs/git.go +++ b/pkg/vcs/git.go @@ -153,7 +153,7 @@ func (git *git) HeadCommit() (*Commit, error) { } func (git *git) getCommit(commit string) (*Commit, error) { - output, err := git.git("log", "--format=%H%n%s%n%ae%n%an%n%ad%n%b", "-n", "1", commit) + output, err := git.git("log", "--format=%H%n%s%n%ae%n%an%n%ad%n%P%n%b", "-n", "1", commit) if err != nil { return nil, err } @@ -182,7 +182,7 @@ func gitParseCommit(output, user, domain []byte, ignoreCC map[string]bool) (*Com cc := make(map[string]bool) cc[strings.ToLower(string(lines[2]))] = true var tags []string - bodyLines := lines[5:] + bodyLines := lines[6:] if isEmpty(bodyLines) { // Body is empty, use summary instead. bodyLines = [][]byte{lines[1]} @@ -231,11 +231,13 @@ func gitParseCommit(output, user, domain []byte, ignoreCC map[string]bool) (*Com sortedCC = append(sortedCC, addr) } sort.Strings(sortedCC) + parents := strings.Split(string(lines[5]), " ") com := &Commit{ Hash: string(lines[0]), Title: string(lines[1]), Author: string(lines[2]), AuthorName: string(lines[3]), + Parents: parents, CC: sortedCC, Tags: tags, Date: date, @@ -303,7 +305,7 @@ func (git *git) ExtractFixTagsFromCommits(baseCommit, email string) ([]*Commit, func (git *git) fetchCommits(since, base, user, domain string, greps []string, fixedStrings bool) ([]*Commit, error) { const commitSeparator = "---===syzkaller-commit-separator===---" - args := []string{"log", "--since", since, "--format=%H%n%s%n%ae%n%an%n%ad%n%b%n" + commitSeparator} + args := []string{"log", "--since", since, "--format=%H%n%s%n%ae%n%an%n%ad%n%P%n%b%n" + commitSeparator} if fixedStrings { args = append(args, "--fixed-strings") } diff --git a/pkg/vcs/git_repo_test.go b/pkg/vcs/git_repo_test.go index 227834b71..e1d317ea2 100644 --- a/pkg/vcs/git_repo_test.go +++ b/pkg/vcs/git_repo_test.go @@ -116,6 +116,7 @@ func TestMetadata(t *testing.T) { } defer os.RemoveAll(repoDir) repo := MakeTestRepo(t, repoDir) + prevHash := "" for i, test := range metadataTests { repo.CommitChange(test.description) com, err := repo.repo.HeadCommit() @@ -123,6 +124,10 @@ func TestMetadata(t *testing.T) { t.Fatal(err) } checkCommit(t, i, test, com, false) + if len(com.Parents) != 1 || com.Parents[0] != prevHash { + t.Fatalf("bad parents: %+q, expect %q", com.Parents, prevHash) + } + prevHash = com.Hash } commits, err := repo.repo.ExtractFixTagsFromCommits("HEAD", extractFixTagsEmail) if err != nil { diff --git a/pkg/vcs/vcs.go b/pkg/vcs/vcs.go index 12130d594..777f0f7e7 100644 --- a/pkg/vcs/vcs.go +++ b/pkg/vcs/vcs.go @@ -74,6 +74,7 @@ type Commit struct { AuthorName string CC []string Tags []string + Parents []string Date time.Time } diff --git a/syz-ci/jobs.go b/syz-ci/jobs.go index b6fb28a07..f6bc01c37 100644 --- a/syz-ci/jobs.go +++ b/syz-ci/jobs.go @@ -397,12 +397,12 @@ func (jp *JobProcessor) bisect(job *Job, mgrcfg *mgrconfig.Config) error { Manager: *mgrcfg, } - commits, rep, com, err := bisect.Run(cfg) + res, err := bisect.Run(cfg) resp.Log = trace.Bytes() if err != nil { return err } - for _, com := range commits { + for _, com := range res.Commits { resp.Commits = append(resp.Commits, dashapi.Commit{ Hash: com.Hash, Title: com.Title, @@ -412,18 +412,26 @@ func (jp *JobProcessor) bisect(job *Job, mgrcfg *mgrconfig.Config) error { Date: com.Date, }) } - if rep != nil { - resp.CrashTitle = rep.Title - resp.CrashReport = rep.Report - resp.CrashLog = rep.Output + if len(res.Commits) == 1 { + if len(res.Commits[0].Parents) > 1 { + resp.Flags |= dashapi.BisectResultMerge + } + if res.NoopChange { + resp.Flags |= dashapi.BisectResultNoop + } + } + if res.Report != nil { + resp.CrashTitle = res.Report.Title + resp.CrashReport = res.Report.Report + resp.CrashLog = res.Report.Output if len(resp.Commits) != 0 { - resp.Commits[0].CC = append(resp.Commits[0].CC, rep.Maintainers...) + resp.Commits[0].CC = append(resp.Commits[0].CC, res.Report.Maintainers...) } else { // If there is a report ahd there is no commit, it means a crash // occurred on HEAD(for BisectFix) and oldest tested release(for BisectCause). - resp.Build.KernelCommit = com.Hash - resp.Build.KernelCommitDate = com.Date - resp.Build.KernelCommitTitle = com.Title + resp.Build.KernelCommit = res.Commit.Hash + resp.Build.KernelCommitDate = res.Commit.Date + resp.Build.KernelCommitTitle = res.Commit.Title } } return nil @@ -475,7 +483,7 @@ func (jp *JobProcessor) testPatch(job *Job, mgrcfg *mgrconfig.Config) error { } log.Logf(0, "job: building kernel...") - kernelConfig, err := env.BuildKernel(mgr.mgrcfg.Compiler, mgr.mgrcfg.Userspace, mgr.mgrcfg.KernelCmdline, + kernelConfig, _, err := env.BuildKernel(mgr.mgrcfg.Compiler, mgr.mgrcfg.Userspace, mgr.mgrcfg.KernelCmdline, mgr.mgrcfg.KernelSysctl, req.KernelConfig) if err != nil { return err diff --git a/tools/syz-bisect/bisect.go b/tools/syz-bisect/bisect.go index 352dda272..fa453c08f 100644 --- a/tools/syz-bisect/bisect.go +++ b/tools/syz-bisect/bisect.go @@ -103,7 +103,7 @@ func main() { loadFile("kernel.config", &cfg.Kernel.Config) loadFile("repro.syz", &cfg.Repro.Syz) loadFile("repro.opts", &cfg.Repro.Opts) - if _, _, _, err := bisect.Run(cfg); err != nil { + if _, err := bisect.Run(cfg); err != nil { fmt.Fprintf(os.Stderr, "bisection failed: %v\n", err) os.Exit(1) } diff --git a/tools/syz-testbuild/testbuild.go b/tools/syz-testbuild/testbuild.go index a4db5be8a..fd5a0fea3 100644 --- a/tools/syz-testbuild/testbuild.go +++ b/tools/syz-testbuild/testbuild.go @@ -119,7 +119,7 @@ func main() { } } -func test(repo vcs.Repo, bisecter vcs.Bisecter, kernelConfig []byte, env instance.BuilderTester, com *vcs.Commit) { +func test(repo vcs.Repo, bisecter vcs.Bisecter, kernelConfig []byte, env instance.Env, com *vcs.Commit) { bisectEnv, err := bisecter.EnvForCommit(*flagBisectBin, com.Hash, kernelConfig) if err != nil { fail(err) @@ -128,7 +128,7 @@ func test(repo vcs.Repo, bisecter vcs.Bisecter, kernelConfig []byte, env instanc if err := build.Clean(*flagOS, *flagArch, vmType, *flagKernelSrc); err != nil { fail(err) } - _, err = env.BuildKernel(bisectEnv.Compiler, *flagUserspace, + _, _, err = env.BuildKernel(bisectEnv.Compiler, *flagUserspace, *flagKernelCmdline, *flagKernelSysctl, bisectEnv.KernelConfig) if err != nil { if verr, ok := err.(*osutil.VerboseError); ok { -- cgit mrf-deployment