hashicorp / hashicorp/consul

data race in lib/serf.UpdateTag

Open
#9,457 0 comments 1 reaction 0 assignees View on GitHub
theme/internals type/bug
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

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. (https://github.com/hashicorp/serf/issues/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
```

Contributor guide

Open the contributing guide

Research direction

Review lib/serf/serf.go, especially UpdateTag, alongside the Serf.SetTags and Serf.GetTags call paths shown in the report. Run the relevant Go tests with the race detector and determine how concurrent tag updates should preserve all tags. Done means the race is no longer reported and concurrent updates do not lose tags.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.