Skip to content

data race in lib/serf.UpdateTag #9457

@dnephin

Description

@dnephin

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. (Serf.SetTags can race with itself 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

Metadata

Metadata

Assignees

No one assigned

    Labels

    theme/internalsSerf, Raft, SWIM, Lifeguard, Anti-Entropy, locking topicstype/bugFeature does not function as expected

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions