dashboard/app: allow testing fixes on exact commit and without patch

This implements 2 features:
1. It's now possible to specify exact commit when testing as:

2. It's possible to test without patch attached
assuming the patch is already committed to the tested tree.

Fixes #558

dashboard/app/api.go

@@ -233,7 +233,8 @@ func apiUploadBuild(c context.Context, ns string, r *http.Request, payload []byt
 	if err := json.Unmarshal(payload, req); err != nil {
 		return nil, fmt.Errorf("failed to unmarshal request: %v", err)
 	}
-	isNewBuild, err := uploadBuild(c, ns, req, BuildNormal)
+	now := timeNow(c)
+	isNewBuild, err := uploadBuild(c, now, ns, req, BuildNormal)
 	if err != nil {
 		return nil, err
 	}
@@ -253,7 +254,7 @@ func apiUploadBuild(c context.Context, ns string, r *http.Request, payload []byt
 	return nil, nil
 }
 
-func uploadBuild(c context.Context, ns string, req *dashapi.Build, typ BuildType) (bool, error) {
+func uploadBuild(c context.Context, now time.Time, ns string, req *dashapi.Build, typ BuildType) (bool, error) {
 	if _, err := loadBuild(c, ns, req.ID); err == nil {
 		return false, nil
 	}
@@ -276,8 +277,8 @@ func uploadBuild(c context.Context, ns string, req *dashapi.Build, typ BuildType
 	if err := checkStrLen(req.KernelRepo, "Build.KernelRepo", MaxStringLen); err != nil {
 		return false, err
 	}
-	if err := checkStrLen(req.KernelBranch, "Build.KernelBranch", MaxStringLen); err != nil {
-		return false, err
+	if len(req.KernelBranch) > MaxStringLen {
+		return false, fmt.Errorf("Build.KernelBranch is too long (%v)", len(req.KernelBranch))
 	}
 	if err := checkStrLen(req.SyzkallerCommit, "Build.SyzkallerCommit", MaxStringLen); err != nil {
 		return false, err
@@ -297,7 +298,7 @@ func uploadBuild(c context.Context, ns string, req *dashapi.Build, typ BuildType
 		Manager:           req.Manager,
 		ID:                req.ID,
 		Type:              typ,
-		Time:              timeNow(c),
+		Time:              now,
 		OS:                req.OS,
 		Arch:              req.Arch,
 		VMArch:            req.VMArch,
@@ -450,7 +451,8 @@ func apiReportBuildError(c context.Context, ns string, r *http.Request, payload
 	if err := json.Unmarshal(payload, req); err != nil {
 		return nil, fmt.Errorf("failed to unmarshal request: %v", err)
 	}
-	if _, err := uploadBuild(c, ns, &req.Build, BuildFailed); err != nil {
+	now := timeNow(c)
+	if _, err := uploadBuild(c, now, ns, &req.Build, BuildFailed); err != nil {
 		return nil, err
 	}
 	req.Crash.BuildID = req.Build.ID

dashboard/app/entities.go

@@ -355,7 +355,10 @@ func kernelRepoInfo(build *Build) KernelRepo {
 }
 
 func kernelRepoInfoRaw(repo, branch string) KernelRepo {
-	repoID := repo + "/" + branch
+	repoID := repo
+	if branch != "" {
+		repoID += "/" + branch
+	}
 	info := config.KernelRepos[repoID]
 	if info.Alias == "" {
 		info.Alias = repoID

dashboard/app/jobs.go

@@ -87,10 +87,8 @@ func addTestJob(c context.Context, bug *Bug, bugKey *datastore.Key, bugReporting
 	switch {
 	case !git.CheckRepoAddress(repo):
 		return fmt.Sprintf("%q does not look like a valid git repo address.", repo), nil
-	case !git.CheckBranch(branch):
-		return fmt.Sprintf("%q does not look like a valid git branch name.", branch), nil
-	case len(patch) == 0:
-		return "I don't see any patch attached to the request.", nil
+	case !git.CheckBranch(branch) && !git.CheckCommitHash(branch):
+		return fmt.Sprintf("%q does not look like a valid git branch or commit.", branch), nil
 	case crash.ReproC == 0 && crash.ReproSyz == 0:
 		return "This crash does not have a reproducer. I cannot test it.", nil
 	case bug.Status == BugStatusFixed:
@@ -164,7 +162,7 @@ func addTestJob(c context.Context, bug *Bug, bugKey *datastore.Key, bugReporting
 		return nil
 	}
 	err = datastore.RunInTransaction(c, tx, &datastore.TransactionOptions{XG: true, Attempts: 30})
-	if deletePatch || err != nil {
+	if patchID != 0 && deletePatch || err != nil {
 		datastore.Delete(c, datastore.NewKey(c, textPatch, "", patchID, nil))
 	}
 	if err != nil {
@@ -268,7 +266,7 @@ func doneJob(c context.Context, req *dashapi.JobDoneReq) error {
 			return fmt.Errorf("job %v: already finished", jobID)
 		}
 		ns := job.Namespace
-		if isNewBuild, err := uploadBuild(c, ns, &req.Build, BuildJob); err != nil {
+		if isNewBuild, err := uploadBuild(c, now, ns, &req.Build, BuildJob); err != nil {
 			return err
 		} else if !isNewBuild {
 			log.Errorf(c, "job %v: duplicate build %v", jobID, req.Build.ID)

dashboard/app/jobs_test.go

@@ -78,11 +78,6 @@ func TestJob(t *testing.T) {
 	c.expectEQ(len(c.emailSink), 1)
 	c.expectEQ(strings.Contains((<-c.emailSink).Body, "does not look like a valid git repo"), true)
 
-	c.incomingEmail(sender, "#syz test: git://git.git/git.git master",
-		EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList}))
-	c.expectEQ(len(c.emailSink), 1)
-	c.expectEQ(strings.Contains((<-c.emailSink).Body, "I don't see any patch attached to the request"), true)
-
 	c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch,
 		EmailOptFrom("\"foo\" <blAcklisteD@dOmain.COM>"))
 	c.expectOK(c.GET("/email_poll"))
@@ -200,7 +195,6 @@ compiler: compiler1
 Patch: %[1]v
 Kernel config: %[2]v
 
-
 `, patchLink, kernelConfigLink)
 		if msg.Body != body {
 			t.Fatalf("got email body:\n%s\n\nwant:\n%s", msg.Body, body)
@@ -246,7 +240,6 @@ compiler: compiler1
 Patch: %[3]v
 Kernel config: %[4]v
 
-
 `, truncatedError, errorLink, patchLink, kernelConfigLink)
 		if msg.Body != body {
 			t.Fatalf("got email body:\n%s\n\nwant:\n%s", msg.Body, body)
@@ -287,7 +280,6 @@ compiler: compiler1
 Patch: %[2]v
 Kernel config: %[3]v
 
-
 ---
 There is no WARRANTY for the result, to the extent permitted by applicable law.
 Except when otherwise stated in writing syzbot provides the result "AS IS"
@@ -307,3 +299,79 @@ correction.
 	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
 	c.expectEQ(pollResp.ID, "")
 }
+
+func TestJobWithoutPatch(t *testing.T) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	build := testBuild(1)
+	c.expectOK(c.API(client2, key2, "upload_build", build, nil))
+
+	crash := testCrash(build, 1)
+	crash.ReproOpts = []byte("repro opts")
+	crash.ReproSyz = []byte("repro syz")
+	crash.ReproC = []byte("repro C")
+	crash.Maintainers = []string{"maintainer@kernel.org"}
+	c.expectOK(c.API(client2, key2, "report_crash", crash, nil))
+
+	c.expectOK(c.GET("/email_poll"))
+	c.expectEQ(len(c.emailSink), 1)
+	sender := (<-c.emailSink).Sender
+	_, extBugID, err := email.RemoveAddrContext(sender)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Test on particular commit and without a patch.
+	c.incomingEmail(sender, "#syz test: git://mygit.com/git.git 5e6a2eea\n", EmailOptMessageID(1))
+	pollResp := new(dashapi.JobPollResp)
+	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
+	testBuild := testBuild(2)
+	testBuild.KernelRepo = "git://mygit.com/git.git"
+	testBuild.KernelBranch = ""
+	testBuild.KernelCommit = "5e6a2eea5e6a2eea5e6a2eea5e6a2eea5e6a2eea"
+	jobDoneReq := &dashapi.JobDoneReq{
+		ID:    pollResp.ID,
+		Build: *testBuild,
+	}
+	c.expectOK(c.API(client2, key2, "job_done", jobDoneReq, nil))
+	c.expectOK(c.GET("/email_poll"))
+	c.expectEQ(len(c.emailSink), 1)
+	{
+		_, dbBuild := c.loadJob(pollResp.ID)
+		kernelConfigLink := externalLink(c.ctx, textKernelConfig, dbBuild.KernelConfig)
+		msg := <-c.emailSink
+		c.expectEQ(len(msg.Attachments), 0)
+		body := fmt.Sprintf(`Hello,
+
+syzbot has tested the proposed patch and the reproducer did not trigger crash:
+
+Reported-and-tested-by: syzbot+%v@testapp.appspotmail.com
+
+Note: the tag will also help syzbot to understand when the bug is fixed.
+
+Tested on git://mygit.com/git.git commit
+5e6a2eea5e6a2eea5e6a2eea5e6a2eea5e6a2eea (Sat Feb 3 04:05:06 0001 +0000)
+kernel_commit_title2
+
+compiler: compiler2
+Kernel config: %[2]v
+
+---
+There is no WARRANTY for the result, to the extent permitted by applicable law.
+Except when otherwise stated in writing syzbot provides the result "AS IS"
+without warranty of any kind, either expressed or implied, but not limited to,
+the implied warranties of merchantability and fittness for a particular purpose.
+The entire risk as to the quality of the result is with you. Should the result
+prove defective, you assume the cost of all necessary servicing, repair or
+correction.
+`, extBugID, kernelConfigLink)
+		if msg.Body != body {
+			t.Fatalf("got email body:\n%s\n\nwant:\n%s", msg.Body, body)
+		}
+		c.checkURLContents(kernelConfigLink, testBuild.KernelConfig)
+	}
+
+	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
+	c.expectEQ(pollResp.ID, "")
+}

dashboard/app/mail_test_result.txt

@@ -24,10 +24,10 @@ Tested on {{.KernelRepo}} commit
 {{.KernelCommitTitle}}{{end}}
 
 compiler: {{.CompilerID}}
-{{if .PatchLink}}Patch: {{.PatchLink}}{{end}}
-{{if .KernelConfigLink}}Kernel config: {{.KernelConfigLink}}{{end}}
-{{if .LogLink}}Raw console output: {{.LogLink}}{{end}}
-{{if and (not .CrashTitle) (not .Error)}}
+{{if .PatchLink}}Patch: {{.PatchLink}}
+{{end}}{{if .KernelConfigLink}}Kernel config: {{.KernelConfigLink}}
+{{end}}{{if .LogLink}}Raw console output: {{.LogLink}}
+{{end}}{{if and (not .CrashTitle) (not .Error)}}
 ---
 There is no WARRANTY for the result, to the extent permitted by applicable law.
 Except when otherwise stated in writing syzbot provides the result "AS IS"

pkg/git/git.go

@@ -57,9 +57,9 @@ func Poll(dir, repo, branch string) (*Commit, error) {
 	return HeadCommit(dir)
 }
 
-// Checkout checkouts the specified repository/branch in dir.
+// CheckoutBranch checkouts the specified repository/branch in dir.
 // It does not fetch history and efficiently supports checkouts of different repos in the same dir.
-func Checkout(dir, repo, branch string) (*Commit, error) {
+func CheckoutBranch(dir, repo, branch string) (*Commit, error) {
 	if _, err := runSandboxed(dir, "git", "reset", "--hard"); err != nil {
 		if err := initRepo(dir); err != nil {
 			return nil, err
@@ -75,21 +75,21 @@ func Checkout(dir, repo, branch string) (*Commit, error) {
 	return HeadCommit(dir)
 }
 
-// CheckoutCommit checkouts the specified repository/branch/commit in dir.
-func CheckoutCommit(dir, repo, branch, commit string) error {
+// CheckoutCommit checkouts the specified repository on the specified commit in dir.
+func CheckoutCommit(dir, repo, commit string) (*Commit, error) {
 	if _, err := runSandboxed(dir, "git", "reset", "--hard"); err != nil {
 		if err := initRepo(dir); err != nil {
-			return err
+			return nil, err
 		}
 	}
-	_, err := runSandboxed(dir, "git", "fetch", repo, branch)
+	_, err := runSandboxed(dir, "git", "fetch", repo)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	if _, err := runSandboxed(dir, "git", "checkout", commit); err != nil {
-		return err
+		return nil, err
 	}
-	return nil
+	return HeadCommit(dir)
 }
 
 func clone(dir, repo, branch string) error {
@@ -322,11 +322,21 @@ func CheckRepoAddress(repo string) bool {
 	return gitRepoRe.MatchString(repo)
 }
 
-var gitRepoRe = regexp.MustCompile(`^(git|ssh|http|https|ftp|ftps)://[a-zA-Z0-9-_]+(\.[a-zA-Z0-9-_]+)+(:[0-9]+)?/[a-zA-Z0-9-_./]+\.git(/)?$`)
-
 // CheckBranch does a best-effort approximate check of a git branch name.
 func CheckBranch(branch string) bool {
 	return gitBranchRe.MatchString(branch)
 }
 
-var gitBranchRe = regexp.MustCompile("^[a-zA-Z0-9-_/.]{2,200}$")
+func CheckCommitHash(hash string) bool {
+	if !gitHashRe.MatchString(hash) {
+		return false
+	}
+	ln := len(hash)
+	return ln == 8 || ln == 10 || ln == 12 || ln == 16 || ln == 20 || ln == 40
+}
+
+var (
+	gitRepoRe   = regexp.MustCompile(`^(git|ssh|http|https|ftp|ftps)://[a-zA-Z0-9-_]+(\.[a-zA-Z0-9-_]+)+(:[0-9]+)?/[a-zA-Z0-9-_./]+\.git(/)?$`)
+	gitBranchRe = regexp.MustCompile("^[a-zA-Z0-9-_/.]{2,200}$")
+	gitHashRe   = regexp.MustCompile("^[a-f0-9]+$")
+)

pkg/git/git_test.go

@@ -66,6 +66,7 @@ func TestCheckBranch(t *testing.T) {
 		{"linux-4.9.y", true},
 		{"abi_spec", true},
 		{"@", false},
+		{"", false},
 	}
 	for _, test := range tests {
 		res := CheckBranch(test.branch)
@@ -75,6 +76,30 @@ func TestCheckBranch(t *testing.T) {
 	}
 }
 
+func TestCheckCommitHash(t *testing.T) {
+	var tests = []struct {
+		hash   string
+		result bool
+	}{
+		{"ff12bea91c22bba93d3ffc3034d813d686bc7eeb", true}, // 40
+		{"eae05cb0aaeae05cb0aa", true},                     // 20
+		{"449dd6984d0eaabb", true},                         // 16
+		{"449dd6984d0e", true},                             // 12
+		{"eae05cb0aa", true},                               // 10
+		{"eae05cb0", true},                                 // 8
+		{"", false},
+		{"aa", false},
+		{"eae05cb0aab", false},
+		{"xxxxxxxx", false},
+	}
+	for _, test := range tests {
+		res := CheckCommitHash(test.hash)
+		if res != test.result {
+			t.Errorf("%v: got %v, want %v", test.hash, res, test.result)
+		}
+	}
+}
+
 func TestExtractFixTags(t *testing.T) {
 	commits, err := extractFixTags(strings.NewReader(extractFixTagsInput), extractFixTagsEmail)
 	if err != nil {

syz-ci/jobs.go

@@ -199,7 +199,7 @@ func (jp *JobProcessor) buildImage(job *Job) error {
 	}
 
 	Logf(0, "job: fetching syzkaller on %v...", req.SyzkallerCommit)
-	err := git.CheckoutCommit(syzkallerDir, jp.syzkallerRepo, jp.syzkallerBranch, req.SyzkallerCommit)
+	_, err := git.CheckoutCommit(syzkallerDir, jp.syzkallerRepo, req.SyzkallerCommit)
 	if err != nil {
 		return fmt.Errorf("failed to checkout syzkaller repo: %v", err)
 	}
@@ -220,9 +220,20 @@ func (jp *JobProcessor) buildImage(job *Job) error {
 	resp.Build.SyzkallerCommit = req.SyzkallerCommit
 
 	Logf(0, "job: fetching kernel...")
-	kernelCommit, err := git.Checkout(kernelDir, req.KernelRepo, req.KernelBranch)
-	if err != nil {
-		return fmt.Errorf("failed to checkout kernel repo: %v", err)
+	var kernelCommit *git.Commit
+	if git.CheckCommitHash(req.KernelBranch) {
+		kernelCommit, err = git.CheckoutCommit(kernelDir, req.KernelRepo, req.KernelBranch)
+		if err != nil {
+			return fmt.Errorf("failed to checkout kernel repo %v on commit %v: %v",
+				req.KernelRepo, req.KernelBranch, err)
+		}
+		resp.Build.KernelBranch = ""
+	} else {
+		kernelCommit, err = git.CheckoutBranch(kernelDir, req.KernelRepo, req.KernelBranch)
+		if err != nil {
+			return fmt.Errorf("failed to checkout kernel repo %v/%v: %v",
+				req.KernelRepo, req.KernelBranch, err)
+		}
 	}
 	resp.Build.KernelCommit = kernelCommit.Hash
 	resp.Build.KernelCommitTitle = kernelCommit.Title
@@ -231,8 +242,10 @@ func (jp *JobProcessor) buildImage(job *Job) error {
 	if err := kernel.Clean(kernelDir); err != nil {
 		return fmt.Errorf("kernel clean failed: %v", err)
 	}
-	if err := git.Patch(kernelDir, req.Patch); err != nil {
-		return err
+	if len(req.Patch) != 0 {
+		if err := git.Patch(kernelDir, req.Patch); err != nil {
+			return err
+		}
 	}
 
 	Logf(0, "job: building kernel...")