From d9393412ccc24ce82dcd6ab8916c966615150edd Mon Sep 17 00:00:00 2001
From: Space Meyer <meyerpatrick@google.com>
Date: Fri, 12 Aug 2022 14:32:53 +0000
Subject: pkg/bisect: prevent crash in bisection after failed repo HEAD query

During a bisection the call stack looks like this:
Starting at env.bisect() in pkg/bisect/bisect.go
bisect() -> Bisect() -> pred() -> test() -> build()

Previously the `results[testRes1.com.Hash]` in pred() could run into a
nil pointer deref and crash, when:
* build() didn't bother to return current in certain error conditions
* test() didn't handle cases where build() couldn't query current

When test() returns an error, the bisection it was called from is
aborted. Hence test() is expected to not simply bubble up recoverable
errors, but instead return a testResult with verdict=vcs.BisectSkip.

I ran into this with a linux branch, where syzkaller could not find any
release tags, running into one of the cases where build() did not return
current, even though it was able to obtain it.
---
 pkg/bisect/bisect.go | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

(limited to 'pkg/bisect')

diff --git a/pkg/bisect/bisect.go b/pkg/bisect/bisect.go
index 3320e7da3..bc442c16a 100644
--- a/pkg/bisect/bisect.go
+++ b/pkg/bisect/bisect.go
@@ -417,13 +417,13 @@ func (env *env) build() (*vcs.Commit, string, error) {
 
 	bisectEnv, err := env.bisecter.EnvForCommit(env.cfg.BinDir, current.Hash, env.kernelConfig)
 	if err != nil {
-		return nil, "", err
+		return current, "", err
 	}
 	env.log("testing commit %v", current.Hash)
 	buildStart := time.Now()
 	mgr := env.cfg.Manager
 	if err := build.Clean(mgr.TargetOS, mgr.TargetVMArch, mgr.Type, mgr.KernelSrc); err != nil {
-		return nil, "", fmt.Errorf("kernel clean failed: %v", err)
+		return current, "", fmt.Errorf("kernel clean failed: %v", err)
 	}
 	kern := &env.cfg.Kernel
 	_, imageDetails, err := env.inst.BuildKernel(bisectEnv.Compiler, env.cfg.Ccache, kern.Userspace,
@@ -438,6 +438,8 @@ func (env *env) build() (*vcs.Commit, string, error) {
 	return current, imageDetails.Signature, err
 }
 
+// Note: When this function returns an error, the bisection it was called from is aborted.
+// Hence recoverable errors must be handled and the callers must treat testResult with care.
 func (env *env) test() (*testResult, error) {
 	cfg := env.cfg
 	if cfg.Timeout != 0 && time.Since(env.startTime) > cfg.Timeout {
@@ -449,6 +451,10 @@ func (env *env) test() (*testResult, error) {
 		com:        current,
 		kernelSign: kernelSign,
 	}
+	if current == nil {
+		// This is not recoverable, as the caller must know which commit to skip.
+		return res, fmt.Errorf("couldn't get repo HEAD: %v", err)
+	}
 	if err != nil {
 		if verr, ok := err.(*osutil.VerboseError); ok {
 			env.log("%v", verr.Title)
-- 
cgit mrf-deployment