search

gobwas / ws

Tiny WebSocket library for Go.
MIT License
6.1k stars 373 forks source link

fix race condition #190

Closed mattn closed 10 months ago

cristaloleg commented 10 months ago

Any details when this happens?

mattn commented 10 months ago

Sorry, I tested https://github.com/nbd-wtf/go-nostr with go test -race 

Read at 0x00c00036acaf by goroutine 113:
  github.com/gobwas/ws/wsflate.(*MessageState).IsCompressed()
      /home/mattn/go/pkg/mod/github.com/gobwas/ws@v1.3.1/wsflate/extension.go:161 +0x74
  github.com/nbd-wtf/go-nostr.(*Connection).WriteMessage()
      /home/mattn/go/src/github.com/nbd-wtf/go-nostr/connection.go:102 +0x47
  github.com/nbd-wtf/go-nostr.(*Relay).Connect.func2()
      /home/mattn/go/src/github.com/nbd-wtf/go-nostr/relay.go:224 +0x1fa

Previous write at 0x00c00036acaf by goroutine 114:
  github.com/gobwas/ws/wsflate.(*MessageState).SetCompressed()
      /home/mattn/go/pkg/mod/github.com/gobwas/ws@v1.3.1/wsflate/extension.go:155 +0x6e
  github.com/gobwas/ws/wsflate.(*MessageState).UnsetBits()
      /home/mattn/go/pkg/mod/github.com/gobwas/ws@v1.3.1/wsflate/extension.go:172 +0x69
  github.com/gobwas/ws/wsutil.(*Reader).NextFrame()
      /home/mattn/go/pkg/mod/github.com/gobwas/ws@v1.3.1/wsutil/reader.go:211 +0x5ea
  github.com/nbd-wtf/go-nostr.(*Connection).ReadMessage()
      /home/mattn/go/src/github.com/nbd-wtf/go-nostr/connection.go:132 +0xa8
  github.com/nbd-wtf/go-nostr.(*Relay).Connect.func3()
      /home/mattn/go/src/github.com/nbd-wtf/go-nostr/relay.go:241 +0x136

IsCompressed and SetCompressed can be race condition.

cristaloleg commented 10 months ago

Link is 404 :(

mattn commented 10 months ago

Sorry, fixed a link

klauspost commented 10 months ago

It seems a bit strange that this can occur, since it would indicate a fundamentally broken code flow somewhere. AFAICT you are handling the *MessageState unsafely.

Slapping a mutex (or using atomic.Bool) seems to just cure the appearance - but it doesn't seem like this should happen in the first place. Fundamentally, the ReadMessage in goroutine 114 shouldn't be affecting the WriteMessage in goroutine 113, and they are clearly pointing to the same *MessageState.

So to me is doesn't seem like this change should be applied - as is at least.

cristaloleg commented 10 months ago

Yes, I agree with @klauspost . Most of the types in the ws module are not thread-safe by design. Fixing only one seems weird for now.

I never used MessageState like this, but... why not have 2 of them, 1 per reader and 1 per writer? (I may be wrong, but AFAIR, I've seen code like this).

mattn commented 10 months ago

@klauspost @cristaloleg Thanks. I'll fix go-nostr to separate msgState for reader/writer. If I still have same issue, I'll file new issue.