environs/config: Validate mutates the config passed to it

[Dave Cheney]

config.Validate() mutates the config object passed to it.

This is a serious problem as it may not have exclusive ownership of the config object. Consider this code

func (obs *EnvironObserver) loop() error {
        for {
                select {
                case <-obs.tomb.Dying():
                        return nil
                case _, ok := <-obs.environWatcher.Changes():
                        if !ok {
                                return watcher.EnsureErr(obs.environWatcher)
                        }
                }
                config, err := obs.st.EnvironConfig()
                if err != nil {
                        logger.Warningf("error reading environment config: %v", err)
                        continue
                }
                environ, err := environs.New(config)
                if err != nil {
                        logger.Warningf("error creating an environment: %v", err)
                        continue
                }
                obs.mu.Lock()
                obs.environ = environ
                obs.mu.Unlock()
        }
}

The EnvironObserver waits for the environment config to change, then it asks it's state object for the config, passes that to environs.New and then updates it's copy of the environment.

However, environs.New calls down a bunch of places then ends up calling config.Validate to check that the config it was passed was in fact valid. At the bottom of validate there is this line

        cfg.defined = ProcessDeprecatedAttributes(cfg.defined)
        return nil
}

which _updates_ the defined values inside the config. Which is a race because the the EnvironObserver does not own the reference to the st.config object, so it cannot safely update portions of it as config.Config is not defined to be thread safe.

But, it gets worse. If obs.st.EnvironConfig() returned a _copy_ of it's environ config, then environs.New would mutate that _copy_ and I am _sure_ there is code that expects this mutation that Validate does to flow into all the other references to environ config.

[William Reade (fwereade)]

AFAICS the EnvironConfig in play here is fresh from the API, and no references are stored anywhere else: in what sense is it not owned by the EnvironObserver at that point?

(That's not to say there aren't a bunch of things wrong there, but I don't see the data race...)

[Dave Cheney (dave-cheney)]

The environ config is not fresh, it comes from the state object that was passed to NewEnvironObserver.

// NewEnvironObserver waits for the environment to have a valid
// environment configuration and returns a new environment observer.
// While waiting for the first environment configuration, it will
// return with tomb.ErrDying if it receives a value on dying.
func NewEnvironObserver(st EnvironConfigObserver) (*EnvironObserver, error) {
        config, err := st.EnvironConfig()
        if err != nil {
                return nil, err
        }
        environ, err := environs.New(config)
        if err != nil {

The config returned from st.EnvironConfig is the State.config field. If you hold a reference to that State value, then you can retrieve the same config that is later retrieved, then mutated,

func (obs *EnvironObserver) loop() error {
        for {
                select {
                case <-obs.tomb.Dying():
                        return nil
                case _, ok := <-obs.environWatcher.Changes():
                        if !ok {
                                return watcher.EnsureErr(obs.environWatcher)
                        }
                }
                config, err := obs.st.EnvironConfig()

[William Reade (fwereade)]

I see that the fakeState in the tests has this problem, but shouldn't that be addressed by always sending a copy? If the tests are somehow depending on changes made by the SUT being propagated back to .config then, yeah, they're just broken.

[Dave Cheney (dave-cheney)]

Thanks william. This does look like a problem in the tests, the real code does this

func (st *State) EnvironConfig() (*config.Config, error) {
        settings, err := readSettings(st, environGlobalKey)
        if err != nil {
                return nil, errors.Trace(err)
        }
        attrs := settings.Map()
        return config.New(config.NoDefaults, attrs)
}

[Anastasia (anastasia-macmood)]

As this is a tech-debt item, I am lowering its Importance.

[Canonical Juju QA Bot (juju-qa-bot)]

This bug has not been updated in 5 years, so we're marking it Expired. If you believe this is incorrect, please update the status.