Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

data race in lib/serf.UpdateTag #9457

Open
dnephin opened this issue Dec 22, 2020 · 0 comments · May be fixed by hashicorp/memberlist#238
Open

data race in lib/serf.UpdateTag #9457

dnephin opened this issue Dec 22, 2020 · 0 comments · May be fixed by hashicorp/memberlist#238
Labels
theme/internals Serf, Raft, SWIM, Lifeguard, Anti-Entropy, locking topics type/bug Feature does not function as expected

Comments

@dnephin
Copy link
Contributor

dnephin commented Dec 22, 2020 •
edited

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
theme/internals Serf, Raft, SWIM, Lifeguard, Anti-Entropy, locking topics type/bug Feature does not function as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix a data race, and add support for fixing another one in Serf hashicorp/memberlist
1 participant
@dnephin