From 80a26020d7cc0f68dd3773dc8b1c5334371709bc Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sat, 26 Sep 2026 16:09:29 +0800 Subject: [PATCH] util: serialize all ViperProxy access so startup cannot hit concurrent map read/write (#11470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * util: serialize every ViperProxy method; stop promoting unlocked viper calls ViperProxy embedded *viper.Viper, so only the five declared methods took the mutex while every promoted call — GetStringMap in backend.LoadConfiguration was the reported crash — touched viper's maps unsynchronized. `weed server` starts the volume server (SetDefault writer) and the master (GetStringMap reader) back to back, and a race build reports the pair on a plain start. The wrapped viper is now a named field: a method must be declared here to exist on the proxy, so unsynchronized access fails at compile time rather than at runtime. Every promoted use in the tree (GetStringMap, GetUint32, GetFloat64, GetDuration, IsSet, AllKeys, Set) gets a locked wrapper; NewViperProxy replaces struct literals for local vipers. GetStringMap deep-copies its result — viper hands back the internal subtree, so iterating it after the lock is released would race the next writer. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * util: take the shared lock while LoadConfiguration merges a config file viper.MergeInConfig rewrites the same maps the proxy serializes; without the lock a merge can race a concurrent SetDefault or reader exactly like the reported startup crash. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * util: deep-copy slice elements in the GetStringMap snapshot A slice of maps inside the returned subtree still shared the inner maps — copy elements recursively so nothing the caller mutates is viper's internal state. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * util: add the missing AutomaticEnv wrapper used by tests sse_reader_test reaches it through GetViper(); without the wrapper the call no longer exists once the viper field stopped being embedded. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * util: return a fresh slice from GetStringSlice A stored []string comes back uncast from viper — the backing array is shared internal state like the GetStringMap subtree, so copy it while holding the lock. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/security/tls.go | 2 +- weed/security/tls_client_cert_test.go | 6 +- weed/util/config.go | 112 +++++++++++++++++++++++--- weed/util/config_test.go | 53 ++++++++++++ 4 files changed, 158 insertions(+), 15 deletions(-) create mode 100644 weed/util/config_test.go diff --git a/weed/security/tls.go b/weed/security/tls.go index 29b0d3c14..4bdf9e89e 100644 --- a/weed/security/tls.go +++ b/weed/security/tls.go @@ -161,7 +161,7 @@ func LoadClientTLSFromFile(configFile string, component string) (grpc.DialOption v.Set(key, filepath.Join(configDir, p)) } } - return LoadClientTLS(&util.ViperProxy{Viper: v}, component), nil + return LoadClientTLS(util.NewViperProxy(v), component), nil } func LoadClientTLS(config *util.ViperProxy, component string) grpc.DialOption { diff --git a/weed/security/tls_client_cert_test.go b/weed/security/tls_client_cert_test.go index 8161a9ee2..7147f2e32 100644 --- a/weed/security/tls_client_cert_test.go +++ b/weed/security/tls_client_cert_test.go @@ -134,7 +134,7 @@ func TestLoadClientTLSPrefersClientCert(t *testing.T) { serverCert, serverKey := ca.issue(t, dir, "server", []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}) clientCert, clientKey := ca.issue(t, dir, "client", []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}) - v := &util.ViperProxy{Viper: viper.New()} + v := util.NewViperProxy(viper.New()) v.Set("grpc.ca", ca.file) v.Set("grpc.master.cert", serverCert) v.Set("grpc.master.key", serverKey) @@ -153,7 +153,7 @@ func TestLoadClientTLSFallsBackToServingCert(t *testing.T) { ca := newTestCA(t, dir) dualCert, dualKey := ca.issue(t, dir, "dual", []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}) - v := &util.ViperProxy{Viper: viper.New()} + v := util.NewViperProxy(viper.New()) v.Set("grpc.ca", ca.file) v.Set("grpc.master.cert", dualCert) v.Set("grpc.master.key", dualKey) @@ -171,7 +171,7 @@ func TestLoadClientTLSServerOnlyEkuRejected(t *testing.T) { ca := newTestCA(t, dir) serverCert, serverKey := ca.issue(t, dir, "server", []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}) - v := &util.ViperProxy{Viper: viper.New()} + v := util.NewViperProxy(viper.New()) v.Set("grpc.ca", ca.file) v.Set("grpc.master.cert", serverCert) v.Set("grpc.master.key", serverKey) diff --git a/weed/util/config.go b/weed/util/config.go index 181b5efa9..533d7134f 100644 --- a/weed/util/config.go +++ b/weed/util/config.go @@ -3,6 +3,7 @@ package util import ( "strings" "sync" + "time" "github.com/spf13/viper" @@ -39,6 +40,10 @@ func LoadSecurityConfiguration() { } func LoadConfiguration(configFileName string, required bool) (loaded bool) { + // MergeInConfig mutates the shared viper that ViperProxy serializes; + // take the same lock so a merge cannot race a reader or SetDefault. + vp.Lock() + defer vp.Unlock() // find a filer store viper.SetConfigName(configFileName) // name of config file (without extension) @@ -72,8 +77,12 @@ func LoadConfiguration(configFileName string, required bool) (loaded bool) { return true } +// ViperProxy serializes access to the global viper. The wrapped Viper is a +// named field, not embedded, so every method must be declared here under the +// mutex — a promoted method would take no lock and race, e.g. GetStringMap +// against a concurrent SetDefault during `weed server` startup. type ViperProxy struct { - *viper.Viper + v *viper.Viper sync.Mutex } @@ -84,42 +93,123 @@ var ( func (vp *ViperProxy) SetDefault(key string, value interface{}) { vp.Lock() defer vp.Unlock() - vp.Viper.SetDefault(key, value) + vp.v.SetDefault(key, value) +} + +func (vp *ViperProxy) AutomaticEnv() { + vp.Lock() + defer vp.Unlock() + vp.v.AutomaticEnv() } func (vp *ViperProxy) GetString(key string) string { vp.Lock() defer vp.Unlock() - return vp.Viper.GetString(key) + return vp.v.GetString(key) } func (vp *ViperProxy) GetBool(key string) bool { vp.Lock() defer vp.Unlock() - return vp.Viper.GetBool(key) + return vp.v.GetBool(key) } func (vp *ViperProxy) GetInt(key string) int { vp.Lock() defer vp.Unlock() - return vp.Viper.GetInt(key) + return vp.v.GetInt(key) } func (vp *ViperProxy) GetStringSlice(key string) []string { vp.Lock() defer vp.Unlock() - return vp.Viper.GetStringSlice(key) + // a []string value comes back uncast — the same shared subtree issue + // GetStringMap has, so hand the caller a copy + return append([]string(nil), vp.v.GetStringSlice(key)...) +} + +func (vp *ViperProxy) GetStringMap(key string) map[string]interface{} { + vp.Lock() + defer vp.Unlock() + // viper hands back its internal subtree, so a caller iterating it after + // the lock is released would race the next SetDefault — copy it out. + return deepCopyStringMap(vp.v.GetStringMap(key)) +} + +func deepCopyStringMap(m map[string]interface{}) map[string]interface{} { + out := make(map[string]interface{}, len(m)) + for k, v := range m { + out[k] = deepCopyValue(v) + } + return out +} + +func deepCopyValue(v interface{}) interface{} { + switch nested := v.(type) { + case map[string]interface{}: + return deepCopyStringMap(nested) + case []interface{}: + out := make([]interface{}, len(nested)) + for i, item := range nested { + out[i] = deepCopyValue(item) + } + return out + default: + return v + } +} + +func (vp *ViperProxy) GetUint32(key string) uint32 { + vp.Lock() + defer vp.Unlock() + return vp.v.GetUint32(key) +} + +func (vp *ViperProxy) GetFloat64(key string) float64 { + vp.Lock() + defer vp.Unlock() + return vp.v.GetFloat64(key) +} + +func (vp *ViperProxy) GetDuration(key string) time.Duration { + vp.Lock() + defer vp.Unlock() + return vp.v.GetDuration(key) +} + +func (vp *ViperProxy) Set(key string, value interface{}) { + vp.Lock() + defer vp.Unlock() + vp.v.Set(key, value) +} + +func (vp *ViperProxy) IsSet(key string) bool { + vp.Lock() + defer vp.Unlock() + return vp.v.IsSet(key) +} + +func (vp *ViperProxy) AllKeys() []string { + vp.Lock() + defer vp.Unlock() + return vp.v.AllKeys() +} + +// NewViperProxy wraps a specific viper — for configuration loaded from a +// source other than the shared instance GetViper returns. +func NewViperProxy(v *viper.Viper) *ViperProxy { + return &ViperProxy{v: v} } func GetViper() *ViperProxy { vp.Lock() defer vp.Unlock() - if vp.Viper == nil { - vp.Viper = viper.GetViper() - vp.AutomaticEnv() - vp.SetEnvPrefix("weed") - vp.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + if vp.v == nil { + vp.v = viper.GetViper() + vp.v.AutomaticEnv() + vp.v.SetEnvPrefix("weed") + vp.v.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) } return vp diff --git a/weed/util/config_test.go b/weed/util/config_test.go new file mode 100644 index 000000000..86a0cd4d0 --- /dev/null +++ b/weed/util/config_test.go @@ -0,0 +1,53 @@ +package util + +import ( + "fmt" + "sync" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" +) + +// `weed server` starts the volume server (SetDefault writers) and the master +// (GetStringMap readers) back to back. Reads and writes must share the proxy +// lock — a race-enabled build flags the interleaving the issue reported. +func TestViperProxySerializesMapReadsAgainstWrites(t *testing.T) { + p := NewViperProxy(viper.New()) + p.SetDefault("storage.backend.a.enabled", true) + + var wg sync.WaitGroup + for i := 0; i < 4; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + for j := 0; j < 200; j++ { + p.SetDefault(fmt.Sprintf("storage.backend.d%d.enabled", j), i) + } + }(i) + } + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 200; j++ { + if got := p.GetStringMap("storage.backend"); len(got) == 0 { + t.Error("GetStringMap returned no keys") + return + } + } + }() + wg.Wait() + + got := p.GetStringMap("storage.backend") + assert.Contains(t, got, "a") +} + +// A proxy over its own viper serves the wrapped instance, for configuration +// loaded from a source other than the shared one. +func TestNewViperProxyWrapsGivenViper(t *testing.T) { + v := viper.New() + v.Set("x.y", "z") + p := NewViperProxy(v) + assert.Equal(t, "z", p.GetString("x.y")) + assert.Equal(t, map[string]interface{}{"y": "z"}, p.GetStringMap("x")) +}