hashicorp/consul

data race in lib/serf.UpdateTag

Opened this issue · 0 comments

There are actually 2 races here.

  1. Serf.SetTags can race with itself. The godoc for the Serf struct claims that all methods should be safe for concurrent use, but there is no lock around SetTags, so this is not the case. I'm not sure if it should be the responsibility of serf or the caller to handle that locking. (hashicorp/serf#621)
  2. Even if the above is fixed, the logic in lib/serf.UpdateTag is to first get the tags from Serf.GetTags, mutate the map, then set the tags. But there is no lock! So if two goroutines attempt to update the tags at the same time, the second SetTags may not include all the tags from the first, and some tags would have effectively been removed.
Write at 0x00c000e2a590 by goroutine 29:
  github.com/hashicorp/serf/serf.(*Serf).SetTags()
      /home/daniel/go/pkg/mod/github.com/hashicorp/serf@v0.9.5/serf/serf.go:620 +0xa4
  github.com/hashicorp/consul/lib/serf.UpdateTag()
      /home/daniel/pers/code/consul/lib/serf/serf.go:48 +0xbb
  github.com/hashicorp/consul/agent/consul.(*Server).updateSerfTags()
      /home/daniel/pers/code/consul/agent/consul/acl_server.go:89 +0x84
  github.com/hashicorp/consul/agent/consul.(*Server).updateACLAdvertisement()
      /home/daniel/pers/code/consul/agent/consul/acl_server.go:102 +0x709
  github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership()
      /home/daniel/pers/code/consul/agent/consul/leader.go:125 +0x6ce

Previous write at 0x00c000e2a590 by goroutine 387:
  github.com/hashicorp/serf/serf.(*Serf).SetTags()
      /home/daniel/go/pkg/mod/github.com/hashicorp/serf@v0.9.5/serf/serf.go:620 +0xa4
  github.com/hashicorp/consul/lib/serf.UpdateTag()
      /home/daniel/pers/code/consul/lib/serf/serf.go:48 +0xbb
  github.com/hashicorp/consul/agent/consul.(*Server).updateSerfTags()
      /home/daniel/pers/code/consul/agent/consul/acl_server.go:89 +0x84
  github.com/hashicorp/consul/agent/consul.(*Server).updateACLAdvertisement()
      /home/daniel/pers/code/consul/agent/consul/acl_server.go:102 +0x113
  github.com/hashicorp/consul/agent/consul.(*Server).establishLeadership()
      /home/daniel/pers/code/consul/agent/consul/leader.go:314 +0xd8
  github.com/hashicorp/consul/agent/consul.(*Server).leaderLoop()
      /home/daniel/pers/code/consul/agent/consul/leader.go:199 +0x10af
  github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership.func1()
      /home/daniel/pers/code/consul/agent/consul/leader.go:91 +0x79

Goroutine 29 (running) created at:
  github.com/hashicorp/consul/agent/consul.NewServer()
      /home/daniel/pers/code/consul/agent/consul/server.go:585 +0x3029
  github.com/hashicorp/consul/agent/consul.newServer()
      /home/daniel/pers/code/consul/agent/consul/server_test.go:296 +0x204
  github.com/hashicorp/consul/agent/consul.testServerWithConfig.func1()
      /home/daniel/pers/code/consul/agent/consul/server_test.go:262 +0xf3
  github.com/hashicorp/consul/sdk/testutil/retry.run.func2()
      /home/daniel/pers/code/consul/sdk/testutil/retry/retry.go:133 +0x71

Goroutine 387 (running) created at:
  github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership()
      /home/daniel/pers/code/consul/agent/consul/leader.go:89 +0x38b