diff --git a/internal/client/daemon.go b/internal/client/daemon.go index ed85910..2940f58 100644 --- a/internal/client/daemon.go +++ b/internal/client/daemon.go @@ -336,5 +336,11 @@ func (daemon *Daemon) areAllRemotesAvailable() bool { func (daemon *Daemon) chooseRemoteConnectionForCppCompilation(cppInFile string) *RemoteConnection { hasher := fnv.New32a() _, _ = hasher.Write([]byte(filepath.Base(cppInFile))) - return daemon.remoteConnections[int(hasher.Sum32())%len(daemon.remoteConnections)] + // Reduce in uint32, not int. Where int is 32 bits (armv7, i386), int(hasher.Sum32()) + // is negative for every hash with the top bit set -- about half of all file names -- + // and Go's % keeps the sign of the dividend, so the index could come out negative + // and panic. With len == 1 it never did (x%1 == 0 for any x), which masked this + // entirely; with len == n a name panics when its negative hash is not a multiple + // of n, i.e. for (n-1)/2n of all names -- a quarter of them at two servers. + return daemon.remoteConnections[hasher.Sum32()%uint32(len(daemon.remoteConnections))] } diff --git a/internal/client/daemon_test.go b/internal/client/daemon_test.go new file mode 100644 index 0000000..2ad1405 --- /dev/null +++ b/internal/client/daemon_test.go @@ -0,0 +1,70 @@ +package client + +import ( + "fmt" + "strconv" + "testing" +) + +func makeDaemonWithRemotes(numRemotes int) *Daemon { + conns := make([]*RemoteConnection, numRemotes) + for i := range conns { + conns[i] = &RemoteConnection{remoteHost: fmt.Sprintf("host%d", i)} + } + return &Daemon{remoteConnections: conns} +} + +// chooseRemoteConnectionForCppCompilation used to reduce the FNV hash in `int`: +// +// daemon.remoteConnections[int(hasher.Sum32())%len(daemon.remoteConnections)] +// +// Where `int` is 32 bits, that conversion is negative for every hash with the top +// bit set -- about half of all file names -- and Go's % keeps the sign of the +// dividend, so the index could come out negative and panic the daemon. It is not +// every such name: with n remotes the result is negative unless the hash is a +// multiple of n, so (n-1)/2n of all names panic, a quarter of them at n == 2. +// A single remote masked it completely, because x%1 == 0 for any x. +// +// This is the arch-specific half of the guard, and it is honest about it: on a +// 64-bit host the old code cannot produce a negative index, so the test would +// pass against the bug and prove nothing. Skip rather than report a green that +// did not exercise anything. Run the suite under GOARCH=386 or GOARCH=arm to +// actually execute this. +func TestChooseRemoteConnectionIndexIsNonNegativeOn32Bit(t *testing.T) { + if strconv.IntSize != 32 { + t.Skipf("int is %d bits here; this regression only reproduces where int is 32 bits (GOARCH=386, GOARCH=arm)", strconv.IntSize) + } + + for _, numRemotes := range []int{2, 3, 4, 5, 8} { + daemon := makeDaemonWithRemotes(numRemotes) + for i := 0; i < 20000; i++ { + cppInFile := fmt.Sprintf("src/file%d.cpp", i) + if remote := daemon.chooseRemoteConnectionForCppCompilation(cppInFile); remote == nil { + t.Fatalf("numRemotes=%d %s: got nil remote", numRemotes, cppInFile) + } + } + } +} + +// The invariant itself, which is worth checking on whatever architecture the +// suite happens to run on: every configured remote is reachable by some file +// name, and no name selects out of range. This one is not a guard against the +// 32-bit bug above -- it cannot be -- so it does not skip. +func TestChooseRemoteConnectionUsesEveryRemote(t *testing.T) { + for _, numRemotes := range []int{1, 2, 3, 4, 5, 8} { + daemon := makeDaemonWithRemotes(numRemotes) + + seen := make(map[string]bool, numRemotes) + for i := 0; i < 20000; i++ { + cppInFile := fmt.Sprintf("src/file%d.cpp", i) + remote := daemon.chooseRemoteConnectionForCppCompilation(cppInFile) + if remote == nil { + t.Fatalf("numRemotes=%d %s: got nil remote", numRemotes, cppInFile) + } + seen[remote.remoteHost] = true + } + if len(seen) != numRemotes { + t.Errorf("numRemotes=%d: only %d of the remotes were ever chosen", numRemotes, len(seen)) + } + } +}