gossip: remove race on client peer id

[kvoli]

The gossip client peerID was accessed outside of the gossip mutex,
leading to concurrent access. Serialize access by acquiring the gossip
mutex.

Fixes: #122435
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[andrewbaptist]

Nice catch!

Interestingly enough we introduced by an attempt to address incorrect unlocking: #108064.
To note, the file was added to the list of files we don't lint on here: #107577. In nogo_config.json is still marked as

            "pkg/gossip/client.go": "flagged by linter, should be evaluated",

I don't think this lint would have helped here though although I'm not positivev. In gossip we are acquiring the g.mu lock but then accessing c.peerIdD, and I don't know if it is possible to know programmatically which lock needed to be held.

[andrewbaptist]

Looking at this a little more, there is at least one more usage of c.peerID outside of the lock in the RunAsyncTask goroutine. That is also not safe, but I don't know the exact situation to hit it. Maybe we can lift that log line to before the RunAsyncTask call (although it might always be 0 before requestGossip is called.

There are also uses of c.addr, but that doesn't appear to change after construction.

Can you also add a comment on client.peerID to note that it can only be accessed while holding the gossip.mu lock.

I think there is also a bug in gossip client.go:333 where it sets this outside of a lock.

[arulajmani]

modulo comment

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)

---

pkg/gossip/client.go line 36 at r1 (raw file):

	createdAt time.Time
	// peerID is the node ID of the peer we're connected to. This is set when we
	// receive a response from the peer. The gossip mu should be held when

I found it a bit surprising that we're using gossip mu here, but then I came across this comment:

cockroach/pkg/gossip/gossip.go

Lines 228 to 229 in 25232a6

	// Note that access to each client's internal state is serialized by the
	// embedded server's mutex. This is surprising!

Is this the "surprise" it's talking about?

---

pkg/gossip/client.go line 335 at r1 (raw file):

					initCh <- struct{}{}
				}
				if peerID == 0 {

We don't seem to be doing anything meaningful with this. Should we just get rid of peerID entirely?

[kvoli]

TYFTR

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani)

---

pkg/gossip/client.go line 36 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I found it a bit surprising that we're using gossip mu here, but then I came across this comment:

cockroach/pkg/gossip/gossip.go

Lines 228 to 229 in 25232a6

	// Note that access to each client's internal state is serialized by the
	// embedded server's mutex. This is surprising!

Is this the "surprise" it's talking about?

Indeed.

---

pkg/gossip/client.go line 335 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

We don't seem to be doing anything meaningful with this. Should we just get rid of peerID entirely?

Nice, I didn't notice -- removed.

[kvoli]

bors r=arulajmani

[craig]

Build succeeded:

• linux_arm64_build
• local_roachtest
• macos_amd64_build
• acceptance
• linux_amd64_fips_build
• check_generated_code
• macos_arm64_build
• linux_amd64_build
• Bazel Essential CI (Cockroach)
• local_roachtest_fips
• examples_orms
• docker_image_amd64
• windows_build
• lint