cmd/strelaypoolsrv: Fix relay shuffling (fixes #6936) (#6935)

When cap(permanentRelays) >= len(permanentRelays) + len(knownRelays),

	append(permanentRelays, knownRelays...)

returns a slice of the array underlying permanentRelays. The subsequent
rand.Shuffle then mixes the permanent and known relays. Sequential
requests may cause strelaypoolsrv to forget its permanent relays. Worse,
concurrent requests may cause shuffling of the same slice on multiple
processors concurrently.

Co-authored-by: greatroar <@>

cmd/strelaypoolsrv/main.go

@@ -314,8 +314,11 @@ func handleRequest(w http.ResponseWriter, r *http.Request) {
 
 func handleGetRequest(rw http.ResponseWriter, r *http.Request) {
 	rw.Header().Set("Content-Type", "application/json; charset=utf-8")
+
 	mut.RLock()
-	relays := append(permanentRelays, knownRelays...)
+	relays := make([]*relay, len(permanentRelays)+len(knownRelays))
+	n := copy(relays, permanentRelays)
+	copy(relays[n:], knownRelays)
 	mut.RUnlock()
 
 	// Shuffle

cmd/strelaypoolsrv/main_test.go

@@ -0,0 +1,67 @@
+// Copyright © 2020 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+)
+
+func init() {
+	for i := 0; i < 10; i++ {
+		u := fmt.Sprintf("permanent%d", i)
+		permanentRelays = append(permanentRelays, &relay{URL: u})
+	}
+
+	knownRelays = []*relay{
+		{URL: "known1"},
+		{URL: "known2"},
+		{URL: "known3"},
+	}
+
+	mut = new(sync.RWMutex)
+}
+
+// Regression test: handleGetRequest should not modify permanentRelays.
+func TestHandleGetRequest(t *testing.T) {
+	needcap := len(permanentRelays) + len(knownRelays)
+	if needcap > cap(permanentRelays) {
+		t.Fatalf("test setup failed: need cap(permanentRelays) >= %d, have %d",
+			needcap, cap(permanentRelays))
+	}
+
+	w := httptest.NewRecorder()
+	w.Body = new(bytes.Buffer)
+	handleGetRequest(w, httptest.NewRequest("GET", "/", nil))
+
+	result := make(map[string][]*relay)
+	err := json.NewDecoder(w.Body).Decode(&result)
+	if err != nil {
+		t.Fatalf("invalid JSON: %v", err)
+	}
+
+	relays := result["relays"]
+	expect, actual := len(knownRelays)+len(permanentRelays), len(relays)
+	if actual != expect {
+		t.Errorf("expected %d relays, got %d", expect, actual)
+	}
+
+	// Check for changes in permanentRelays.
+	for i, r := range permanentRelays {
+		switch {
+		case !strings.HasPrefix(r.URL, "permanent"):
+			t.Errorf("relay %q among permanent relays", r.URL)
+		case r.URL != fmt.Sprintf("permanent%d", i):
+			t.Error("order of permanent relays changed")
+		}
+	}
+}