connmgr: Concurrent Map Misuse

[p-shahi]

Description

Reported in: ipfs/kubo#9374 (comment)

When running Kubo 0.16.0 users report a crash.
There is a provided stacktrace:

WebUI: http://127.0.0.1:5001/webui
Gateway (readonly) server listening on /ip4/127.0.0.1/tcp/8080
Daemon is ready
fatal error: concurrent map iteration and map write

goroutine 222 [running]:
github.com/libp2p/go-libp2p/p2p/net/connmgr.peerInfos.SortByValueAndStreams.func1.1(0xc0179d00f0?)
        github.com/libp2p/[email protected]/p2p/net/connmgr/connmgr.go:267 +0x6a
github.com/libp2p/go-libp2p/p2p/net/connmgr.peerInfos.SortByValueAndStreams.func1(0x16b?, 0x21b?)
        github.com/libp2p/[email protected]/p2p/net/connmgr/connmgr.go:277 +0x127
sort.partition_func({0xc0119d9370?, 0xc003e967e0?}, 0x0, 0x2d4, 0x32?)
        sort/zsortfunc.go:157 +0x191

https://github.com/libp2p/go-libp2p/blob/master/p2p/net/connmgr/connmgr.go#L255

Versions affected

Kubo 0.16.0 uses go-libp2p v0.23.2
But this issue could have existed for some time.

Root Cause Analysis

<Once determined, fill RCA here>

Open Questions:

If it's valid that concurrent goroutines need to read peerInfos as it's being written to, shouldn't it be locked inside the sync read write mutex?

go-libp2p/p2p/net/connmgr/connmgr.go

Lines 374 to 377 in 7465a50

	cm.plk.RUnlock()
	// Sort peers according to their value.
	candidates.SortByValueAndStreams(true)

go-libp2p/p2p/net/connmgr/connmgr.go

Lines 405 to 407 in 7465a50

	cm.plk.RUnlock()
	candidates.SortByValueAndStreams(true)

go-libp2p/p2p/net/connmgr/connmgr.go

Lines 456 to 468 in 7465a50

	cm.plk.RUnlock()
	if ncandidates < cm.cfg.lowWater {
	log.Info("open connection count above limit but too many are in the grace period")
	// We have too many connections but fewer than lowWater
	// connections out of the grace period.
	//
	// If we trimmed now, we'd kill potentially useful connections.
	return nil
	}
	// Sort peers according to their value.
	candidates.SortByValueAndStreams(false)

[p-shahi]

Some more logs shared here: ipfs/kubo#9374 (comment)

@julian88110 @MarcoPolo would either of you like to take a look?
If the Kubo team wants this in 0.17.0 (ipfs/kubo#9319 (comment)) we may need to do another patch release

[julian88110]

taking a look.

[BigLep]

Thanks @p-shahi being on top of this. We would like to get this in the next (0.17) Kubo release. The RC is on Thursday, 2022-11-10. A patch here could come after the RC but before the final release on 2022-11-17.

[MarcoPolo]

Let's sync up @julian88110 I started this earlier today.

[MarcoPolo]

Synced up. I'll take.