From 0941ce76b7c148fe590a6263a4bc8a32c0b3e807 Mon Sep 17 00:00:00 2001
From: greatroar <61184462+greatroar@users.noreply.github.com>
Date: Thu, 27 Aug 2020 15:51:58 +0200
Subject: [PATCH] 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      |  5 ++-
 cmd/strelaypoolsrv/main_test.go | 67 +++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 cmd/strelaypoolsrv/main_test.go

diff --git a/cmd/strelaypoolsrv/main.go b/cmd/strelaypoolsrv/main.go
index 5ec000fa2..17b668e68 100644
--- a/cmd/strelaypoolsrv/main.go
+++ b/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
diff --git a/cmd/strelaypoolsrv/main_test.go b/cmd/strelaypoolsrv/main_test.go
new file mode 100644
index 000000000..6b5732f12
--- /dev/null
+++ b/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")
+		}
+	}
+}