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.

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

server+addrmgr: add GetLastAttempt to fix race condition in connmgr #1490

Closed
wants to merge 1 commit into from

Conversation

Crypt-iQ
Copy link
Collaborator

Happens because ka.LastAttempt() is called without a mutex while the lastAttempt field is being updated by the addrmanager via Attempt. Can happen if two individual NewConnReq goroutines are spawned and are trying the same address.

==================
WARNING: DATA RACE
Write at 0x00c014f829d8 by goroutine 67:
  github.com/btcsuite/btcd/addrmgr.(*AddrManager).Attempt()
      /Users/nsa/go/src/github.com/btcsuite/btcd/addrmgr/addrmanager.go:861 +0x1b3
  main.newServer.func5()
      /Users/nsa/go/src/github.com/btcsuite/btcd/server.go:2822 +0x26f
  github.com/btcsuite/btcd/connmgr.(*ConnManager).NewConnReq()
      /Users/nsa/go/src/github.com/btcsuite/btcd/connmgr/connmanager.go:391 +0x3e6

Previous read at 0x00c014f829d8 by goroutine 69:
  main.newServer.func5()
      /Users/nsa/go/src/github.com/btcsuite/btcd/addrmgr/knownaddress.go:33 +0x498
  github.com/btcsuite/btcd/connmgr.(*ConnManager).NewConnReq()
      /Users/nsa/go/src/github.com/btcsuite/btcd/connmgr/connmanager.go:391 +0x3e6

Goroutine 67 (running) created at:
  github.com/btcsuite/btcd/connmgr.(*ConnManager).Start()
      /Users/nsa/go/src/github.com/btcsuite/btcd/connmgr/connmanager.go:530 +0x170

Goroutine 69 (running) created at:
  github.com/btcsuite/btcd/connmgr.(*ConnManager).Start()
      /Users/nsa/go/src/github.com/btcsuite/btcd/connmgr/connmanager.go:530 +0x170

a.mtx.Lock()
defer a.mtx.Unlock()

ka := a.find(addr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about instead accepting the KnownAddress directly? Alternative, can we instead protect LastAttempt with a mutex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both would work. Passing in KnownAddress would be easier since the addressmanager already has a mutex, but protecting LastAttempt with a mutex would be better stylistically. I think I'll do the latter

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll need to dig into the code a bit more. It seems that a lot of the members of KnownAddress can be racy, and the same goes for NetAddress. Think I need to figure out the best way to patch in one sweep.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has there been any progress on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to figure out all the fields that race

@jakesylvestre
Copy link
Collaborator

@jcvernaleo (as per #1530)

  • high priority (anecdotally, I've run into this as well and it crashes the routine)
  • bug

@Crypt-iQ Crypt-iQ closed this Dec 17, 2021
@Crypt-iQ Crypt-iQ deleted the addrmgr_race_1109 branch December 17, 2021 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants