From 87b6615cf5b27a4ff0bf9d287e301689b999f91b Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 23 Oct 2018 10:03:36 -0700 Subject: [PATCH] fix config data race This fixes a data-race in the config. This does not fix https://github.com/ipfs/go-ipfs/issues/4942 as there's still a logical race: parallel config updates clobber each other. License: MIT Signed-off-by: Steven Allen --- core/commands/config.go | 9 ++++++--- repo/fsrepo/fsrepo.go | 11 ++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/core/commands/config.go b/core/commands/config.go index 0bed4a8c7cab..999953389e63 100644 --- a/core/commands/config.go +++ b/core/commands/config.go @@ -407,9 +407,12 @@ func transformConfig(configRoot string, configName string, transformer config.Tr } // make a copy to avoid updating repo's config unintentionally - oldCfg := *cfg - newCfg := oldCfg - err = transformer(&newCfg) + cfg, err = cfg.Clone() + if err != nil { + return nil, nil, err + } + + err = transformer(cfg) if err != nil { return nil, nil, err } diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 0567ecc066a8..8b9d43846375 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -476,9 +476,11 @@ func (r *FSRepo) Close() error { return r.lockfile.Close() } +// Config the current config. This function DOES NOT copy the config. The caller +// MUST NOT modify it without first calling `Clone`. +// // Result when not Open is undefined. The method may panic if it pleases. func (r *FSRepo) Config() (*config.Config, error) { - // It is not necessary to hold the package lock since the repo is in an // opened state. The package lock is _not_ meant to ensure that the repo is // thread-safe. The package lock is only meant to guard against removal and @@ -546,11 +548,14 @@ func (r *FSRepo) setConfigUnsynced(updated *config.Config) error { if err := serialize.WriteConfigFile(configFilename, mapconf); err != nil { return err } - *r.config = *updated // copy so caller cannot modify this private config + // Do not use `*r.config = ...`. This will modify the *shared* config + // returned by `r.Config`. + r.config = updated return nil } -// SetConfig updates the FSRepo's config. +// SetConfig updates the FSRepo's config. The user must not modify the config +// object after calling this method. func (r *FSRepo) SetConfig(updated *config.Config) error { // packageLock is held to provide thread-safety.