commit cf3851c6c9380368373ac127cde1f4aa7159fba3
author Adam Langley <agl@google.com> Tue May 23 00:07:03 2023 +0000
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue May 23 14:00:53 2023 +0000
tree 3167f83edd18eba54a41c70d31b28e5980cc25ca
parent f712c86eda36a59c5939879edda811c771990241

acvptool: clean up better.

The Close() method of the middle often wasn't getting called because
`os.Exit(0)` was used in some places. Once that's fixed, it's clear that
the queue of pending reads needed to be closed before waiting for the
reader goroutine to finish. Lastly, don't bother trying to record the
error that the reader saw: just panic the process if the modulewrapper
dies during processing.

Change-Id: Icf077cefd0ace2ef721a493f99fede6269531257
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60045
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

util/fipstools/acvp/acvptool/acvp.go

diff --git a/util/fipstools/acvp/acvptool/acvp.go b/util/fipstools/acvp/acvptool/acvp.go
index e92424b..cbb07f6 100644
--- a/util/fipstools/acvp/acvptool/acvp.go
+++ b/util/fipstools/acvp/acvptool/acvp.go
@@ -598,14 +598,14 @@
 		}
 		os.Stdout.Write(regcapBytes)
 		os.Stdout.WriteString("\n")
-		os.Exit(0)
+		return
 	}
 
 	if len(*jsonInputFile) > 0 {
 		if err := processFile(*jsonInputFile, supportedAlgos, middle); err != nil {
 			log.Fatalf("failed to process input file: %s", err)
 		}
-		os.Exit(0)
+		return
 	}
 
 	var requestedAlgosFlag string
@@ -789,7 +789,7 @@
 
 	if len(*fetchFlag) > 0 {
 		io.WriteString(fetchOutputTee, "]\n")
-		os.Exit(0)
+		return
 	}
 
 	if ok, err := getResultsWithRetry(server, url); err != nil {

util/fipstools/acvp/acvptool/subprocess/subprocess.go

diff --git a/util/fipstools/acvp/acvptool/subprocess/subprocess.go b/util/fipstools/acvp/acvptool/subprocess/subprocess.go
index 9167b47..f1cb5fa 100644
--- a/util/fipstools/acvp/acvptool/subprocess/subprocess.go
+++ b/util/fipstools/acvp/acvptool/subprocess/subprocess.go
@@ -48,8 +48,6 @@
 	pendingReads chan pendingRead
 	// readerFinished is a channel that is closed if `readerRoutine` has finished (e.g. because of a read error).
 	readerFinished chan struct{}
-	// readerError is set iff readerFinished is closed. If non-nil then it is the read error that caused `readerRoutine` to finished.
-	readerError error
 }
 
 // pendingRead represents an expected response from the modulewrapper.
@@ -153,6 +151,7 @@
 	m.stdout.Close()
 	m.stdin.Close()
 	m.cmd.Wait()
+	close(m.pendingReads)
 	<-m.readerFinished
 }
 
@@ -176,7 +175,7 @@
 func (m *Subprocess) enqueueRead(pending pendingRead) error {
 	select {
 	case <-m.readerFinished:
-		return m.readerError
+		panic("attempted to enqueue request after the reader failed")
 	default:
 	}
 
@@ -266,7 +265,7 @@
 	case <-done:
 		return result, nil
 	case <-m.readerFinished:
-		return nil, m.readerError
+		panic("was still waiting for a result when the reader finished")
 	}
 }
 
@@ -284,13 +283,11 @@
 
 		result, err := m.readResult(pendingRead.cmd, pendingRead.expectedNumResults)
 		if err != nil {
-			m.readerError = err
-			return
+			panic(fmt.Errorf("failed to read from subprocess: %w", err))
 		}
 
 		if err := pendingRead.callback(result); err != nil {
-			m.readerError = err
-			return
+			panic(fmt.Errorf("result from subprocess was rejected: %w", err))
 		}
 	}
 }