config: move locking to fix fatal error: concurrent map read and map write

Before this change we assumed that github.com/Unknwon/goconfig was
threadsafe as documented.

However it turns out it is not threadsafe and looking at the code it
appears that making it threadsafe might be quite hard.

So this change increases the lock coverage in configfile to cover the
goconfig uses also.

Fixes #6378
This commit is contained in:
Nick Craig-Wood 2022-08-13 11:59:42 +01:00
parent 603efbfe76
commit 50657752fd

View File

@ -24,16 +24,15 @@ func Install() {
// Storage implements config.Storage for saving and loading config
// data in a simple INI based file.
type Storage struct {
gc *goconfig.ConfigFile // config file loaded - thread safe
mu sync.Mutex // to protect the following variables
gc *goconfig.ConfigFile // config file loaded - not thread safe
fi os.FileInfo // stat of the file when last loaded
}
// Check to see if we need to reload the config
func (s *Storage) check() {
s.mu.Lock()
defer s.mu.Unlock()
//
// mu must be held when calling this
func (s *Storage) _check() {
if configPath := config.GetConfigPath(); configPath != "" {
// Check to see if config file has changed since it was last loaded
fi, err := os.Stat(configPath)
@ -174,7 +173,10 @@ func (s *Storage) Save() error {
// Serialize the config into a string
func (s *Storage) Serialize() (string, error) {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
var buf bytes.Buffer
if err := goconfig.SaveConfigData(s.gc, &buf); err != nil {
return "", fmt.Errorf("failed to save config file: %w", err)
@ -185,7 +187,10 @@ func (s *Storage) Serialize() (string, error) {
// HasSection returns true if section exists in the config file
func (s *Storage) HasSection(section string) bool {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
_, err := s.gc.GetSection(section)
return err == nil
}
@ -193,26 +198,38 @@ func (s *Storage) HasSection(section string) bool {
// DeleteSection removes the named section and all config from the
// config file
func (s *Storage) DeleteSection(section string) {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
s.gc.DeleteSection(section)
}
// GetSectionList returns a slice of strings with names for all the
// sections
func (s *Storage) GetSectionList() []string {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
return s.gc.GetSectionList()
}
// GetKeyList returns the keys in this section
func (s *Storage) GetKeyList(section string) []string {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
return s.gc.GetKeyList(section)
}
// GetValue returns the key in section with a found flag
func (s *Storage) GetValue(section string, key string) (value string, found bool) {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
value, err := s.gc.GetValue(section, key)
if err != nil {
return "", false
@ -222,7 +239,10 @@ func (s *Storage) GetValue(section string, key string) (value string, found bool
// SetValue sets the value under key in section
func (s *Storage) SetValue(section string, key string, value string) {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
if strings.HasPrefix(section, ":") {
fs.Logf(nil, "Can't save config %q for on the fly backend %q", key, section)
return
@ -232,7 +252,10 @@ func (s *Storage) SetValue(section string, key string, value string) {
// DeleteKey removes the key under section
func (s *Storage) DeleteKey(section string, key string) bool {
s.check()
s.mu.Lock()
defer s.mu.Unlock()
s._check()
return s.gc.DeleteKey(section, key)
}