Viper: UnmarshalKey function is not safe for concurrent access.

[lnashier]

Config json

{
   "someKey": 1,
   "options": {
       "x": "v1",
       "y": "v2"
   }
}

Some struct:

type Options struct {
  X string `json:"x"`
  Y string `json:"y"`
}

Sudo code:

config := viper.New()
config.AddConfigPath("myconfigpath")
config.SetConfigName("myconfig")

go func() {
 for {
    config.getString("someKey")
 }
}()

go func() {
 for {
   var opt Options
   config.UnmarshalKey("options", &opt)
 }
}()

Fatal error:

fatal error: concurrent map read and map write

This error happens due to https://github.com/spf13/viper/blob/master/util.go#L84
I would like to ask purpose of normalizing the keys here.

Normalization of keys is also the reason for #373

[lnashier]

Removed all strings.ToLower function calls from appropriate places. I did not see any issues.

I am curious what's the reason to make keys case-insensitive. @spf13

but did not remove from places like switch strings.ToLower(v.getConfigType()) https://github.com/spf13/viper/blob/master/viper.go#L1275

[lnashier]

We did a POC: https://github.com/lnashier/viper

Concurrent issue is resolved. We didn't see any issue with any get functions or any other functions.

[xvello]

We are also triggering this race condition, leading to a panic:

fatal error: concurrent map read and map write

goroutine 1 [running]:
runtime.throw(0x1f77696, 0x21)
  /root/.gimme/versions/go1.9.4.linux.amd64/src/runtime/panic.go:605 +0x95 fp=0xc4205719b0 sp=0xc420571990 pc=0x44bc05
runtime.mapaccess2_faststr(0x1c81a60, 0xc420067d70, 0x1f549c3, 0xc, 0xc42014ec70, 0x1)
  /root/.gimme/versions/go1.9.4.linux.amd64/src/runtime/hashmap_fast.go:324 +0x47a fp=0xc420571a08 sp=0xc4205719b0 pc=0x42c0aa
github.com/DataDog/datadog-agent/vendor/github.com/spf13/viper.(*Viper).searchMap(0xc42007e1e0, 0xc420067d70, 0xc42014ec70, 0x1, 0x1, 0x1, 0x1)
  vendor/github.com/spf13/viper/viper.go:422 +0x5f fp=0xc420571a50 sp=0xc420571a08 pc=0x880d7f
github.com/DataDog/datadog-agent/vendor/github.com/spf13/viper.(*Viper).Get(0xc42007e1e0, 0x1f549c3, 0xc, 0xc420477680, 0xc420477680)
  vendor/github.com/spf13/viper/viper.go:603 +0x130 fp=0xc420571b10 sp=0xc420571a50 pc=0x881780
github.com/DataDog/datadog-agent/vendor/github.com/spf13/viper.(*Viper).GetBool(0xc42007e1e0, 0x1f549c3, 0xc, 0x0)
  vendor/github.com/spf13/viper/viper.go:655 +0x3f fp=0xc420571b48 sp=0xc420571b10 pc=0x88220f

goroutine 14 [runnable]:
unicode.ToLower(0xc400000072, 0x6f)
  /root/.gimme/versions/go1.9.4.linux.amd64/src/unicode/letter.go:262 +0x67
strings.Map(0x1ff7b58, 0x1f722a7, 0x1e, 0x2fe3b70, 0xc42001ea40)
  /root/.gimme/versions/go1.9.4.linux.amd64/src/strings/strings.go:518 +0x83
strings.ToLower(0x1f722a7, 0x1e, 0x1f6121d, 0x14)
  /root/.gimme/versions/go1.9.4.linux.amd64/src/strings/strings.go:605 +0x41
github.com/DataDog/datadog-agent/vendor/github.com/spf13/viper.insensitiviseMap(0xc420067d70)
  vendor/github.com/spf13/viper/util.go:87 +0x117
github.com/DataDog/datadog-agent/vendor/github.com/spf13/viper.(*Viper).insensitiviseMaps(0xc42007e1e0)
  vendor/github.com/spf13/viper/viper.go:1302 +0x47
github.com/DataDog/datadog-agent/vendor/github.com/spf13/viper.(*Viper).UnmarshalKey(0xc42007e1e0, 0x1f4b1d6, 0x5, 0x1b1fe80, 0xc420284180, 0x4f0b64, 0xc42026c000)
  vendor/github.com/spf13/viper/viper.go:729 +0xb8

Looking at UnmarshalKey, I do not see the impact v.insensitiviseMaps() would have on the return value, as we already finished decoding. Are we intending to run insensitiviseMap(rawVal) instead? @bep some insights would be useful here.

@lnashier although making lookups case-insensitive is indeed pretty counter-intuitive, I do think changing the behaviour might break several use cases (unknowingly?) depending on it. Do you plan on opening a PR to address this race condition?

[mritd]

Is the problem still not progressing?

[stanislav-bios-baranov]

Facing same issue:

Here is small test, which fails for me in 50% of cases:

package main

import (
    "fmt"
    "testing"

    "github.com/stretchr/testify/require"

    "github.com/spf13/afero"
    "github.com/spf13/viper"
)

type Setting struct {
    TestValue *string `json:"testValue,omitempty" mapstructure:"testvalue"`
}

func TestConcurrency(t *testing.T) {

    content := []byte(`{"settings": {"A": {"value": "value A"}, "B": {"value": "value B"}}}`)

    aferoFS := afero.NewMemMapFs()
    require.NoError(t, afero.WriteFile(aferoFS, "conf.json", content, 0755))

    v := viper.New()

    v.SetConfigType("json")
    v.SetFs(aferoFS)
    v.SetConfigFile("conf.json")
    require.NoError(t, v.ReadInConfig())

    for i := 1; i < 100; i++ {
        t.Run(fmt.Sprintf("flow #%d", i), func(tt *testing.T) {
            tt.Parallel()
            settings := make(map[string]*Setting)
            require.NotPanics(t, func() {
                require.NoError(t, v.UnmarshalKey("settings", &settings))
            })
        })
    }

}

[bep]

Why do you need this to run in multiple Go routines?

[stanislav-bios-baranov]

F.I. microservice with HTTP REST API. Configuration is loaded dynamically from config file or from env. vars. Config is reread on on each HTTP request receive.

P.S.: test itself is only to stable reproduce an issue.

[bep]

I have pushed a fixed for this in https://github.com/spf13/viper/pull/666

The original code looks obviously wrong, and there are no test failures. I didn't implement this originally, but I have reviewed this myself, and cannot see any side effects... But if some of you could have a look at it too and give it a thumbs up or something, then I'll merge it.

[lnashier]

Thanks @bep