Skip to content

fix(getter): race condition when SetClient is called#466

Open
srevinsaju wants to merge 1 commit intohashicorp:mainfrom
srevinsaju:fix/srevin/race-set-client
Open

fix(getter): race condition when SetClient is called#466
srevinsaju wants to merge 1 commit intohashicorp:mainfrom
srevinsaju:fix/srevin/race-set-client

Conversation

@srevinsaju
Copy link

This is a non-breaking change. This fixes an issue when Clients are created and called concurrently.
Clients called SetClient on each of the Getters. When no getters were provided, it modified the clients of the default getters defined in the Getters map.
Since the map was mutated from multiple goroutines, it creates a race condition 'concurrent map writes'. The issue is mitigated by creating a new copy of Getters using DefaultGetters() function for every new client created. SetClient now modifies its own copy of the Getter, not the default Getters.

To reproduce the issue:

mkdir hello
touch hello/world
// main.go
package main

import "github.com/hashicorp/go-getter"
import "context"
import "os"
import "sync"
import "path/filepath"

func main() {
    var wg sync.WaitGroup
    cwd, err := os.Getwd()
    if err != nil {
        panic(err)
    }
    keys := []string{"a", "b", "c"}

    for _, key := range keys {
        wg.Add(1)
        go func(key string) {
            get := &getter.Client{
                Ctx: context.Background(),
                Src: "./hello",
                Dst: filepath.Join("dest", key),
                Pwd: cwd,
                Dir: true,
            }
      err := get.Get()
            if err != nil {
                panic(err)
            }
            wg.Done()
        }(key)
    }
    wg.Wait()
}
go run -race .

this is a non-breaking change. this fixes an issue when Clients are created
and called concurrently. clients called SetClient on each of the
Getters. When no getters were provided, it modified the clients
of the default getters defined in a map. Since the map was mutated
from multiple goroutines, it creates a race condition 'concurrent map writes'.
The issue is mitigated by creating a new copy of Getters for every new client
created. SetClient now modifies its own copy of the Getter, not the default Getters
@hashicorp-cla
Copy link

hashicorp-cla commented Oct 23, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@CreatorHead CreatorHead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, the race is real. One concern: this changes behavior when Client.Getters is nil. Previously we used the package-level Getters map, which many consumers may customize (e.g. adding schemes). With c.Getters = DefaultGetters() we would ignore those customizations. Can we instead copy the global Getters map into a per-client map (so we avoid shared mutation, but still honor user overrides)? Also, could you add a small test that reproduces the race with -race and verifies the fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments