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

Data race on sshContext #160

Closed
prologic opened this issue Aug 31, 2021 · 10 comments · Fixed by #211
Closed

Data race on sshContext #160

prologic opened this issue Aug 31, 2021 · 10 comments · Fixed by #211

Comments

@prologic
Copy link

came across this data race:

==================
WARNING: DATA RACE
Write at 0x00c0000988d0 by goroutine 14:
  github.com/gliderlabs/ssh.(*sshContext).SetValue()
      /Users/prologic/go/pkg/mod/github.com/gliderlabs/[email protected]/context.go:123 +0xa6
  github.com/gliderlabs/ssh.(*Server).config.func2()
      /Users/prologic/go/pkg/mod/github.com/gliderlabs/[email protected]/server.go:150 +0x18b
  golang.org/x/crypto/ssh.(*connection).serverAuthenticate()
      /Users/prologic/go/pkg/mod/golang.org/x/[email protected]/ssh/server.go:523 +0x2799
  golang.org/x/crypto/ssh.(*connection).serverHandshake()
      /Users/prologic/go/pkg/mod/golang.org/x/[email protected]/ssh/server.go:277 +0xb78
  golang.org/x/crypto/ssh.NewServerConn()
      /Users/prologic/go/pkg/mod/golang.org/x/[email protected]/ssh/server.go:206 +0x2ab
  github.com/gliderlabs/ssh.(*Server).HandleConn()
      /Users/prologic/go/pkg/mod/github.com/gliderlabs/[email protected]/server.go:281 +0x2af

Previous read at 0x00c0000988d0 by goroutine 39:
  github.com/gliderlabs/ssh.(*sshContext).Done()
      <autogenerated>:1 +0x48
  net/http.http2awaitRequestCancel()
      /usr/local/Cellar/go/1.16.6/libexec/src/net/http/h2_bundle.go:6823 +0x117
  net/http.(*http2clientStream).awaitRequestCancel()
      /usr/local/Cellar/go/1.16.6/libexec/src/net/http/h2_bundle.go:6846 +0x64
@belak
Copy link
Collaborator

belak commented May 5, 2022

Do you have sample code, or was this when running tests? Unfortunately, unless we happen to run across the same race, there's no way to debug this.

Also, I just noticed that this is coming from inside an http handler, so there's a larger likelihood that it's an issue with your code somewhere.

@belak
Copy link
Collaborator

belak commented May 10, 2022

We haven't gotten a response in almost a week, so I'm closing this for now. Feel free to comment with more information or file a new issue and we'll do our best to address this.

@belak belak closed this as completed May 10, 2022
@prologic
Copy link
Author

We haven't gotten a response in almost a week, so I'm closing this for now. Feel free to comment with more information or file a new issue and we'll do our best to address this.

Oh I'm so sorry! This issue got lost in the "ether" for me. Can we please reopen it, and I'll get you a reproducer ASAP 👌

@belak belak reopened this May 10, 2022
@prologic
Copy link
Author

I cannot reproduce this anymore. I may have filed it in error, but nearly a year ago, so I'm not completely sure. Sorry! I ran go test -race . on this repo at v0.3.3 and the latester master -- no problems 👌

@aymanbagabas
Copy link
Contributor

This issue still persists :/

@belak
Copy link
Collaborator

belak commented Jul 12, 2023

@aymanbagabas Where are you seeing this data race? Do you have a way of reproducing it?

@aymanbagabas
Copy link
Contributor

@belak I don't have a MRE right now, but I'm noticing this issue here https://github.com/charmbracelet/soft-serve/actions/runs/5535584234/jobs/10102073239?pr=337#step:4:279

aymanbagabas added a commit to aymanbagabas/ssh that referenced this issue Jul 12, 2023
Context may involve in a RW data race when setting and accessing a value
and its done channel.

Fixes: gliderlabs#160
@aymanbagabas
Copy link
Contributor

@belak This patch works for me #209

aymanbagabas added a commit to aymanbagabas/ssh that referenced this issue Jul 12, 2023
Context may involve in a RW data race when setting and accessing a value
and its done channel.

Fixes: gliderlabs#160
aymanbagabas added a commit to charmbracelet/ssh that referenced this issue Jul 12, 2023
Context may involve in a RW data race when setting and accessing a value
and its done channel.

Fixes: gliderlabs#160
@belak belak reopened this Jul 12, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 9, 2023

Quote:

The fix (charmbracelet#4 / #209) is incomplete.
Every call to sshCtx.Context (including Err() and others) all causes Data Race.

Real example:

==================
WARNING: DATA RACE
Read at 0x00c0065901e0 by goroutine 115717:
  github.com/gliderlabs/ssh.(*sshContext).Err()
      <autogenerated>:1 +0x31
  database/sql.(*Rows).awaitDone()
      /opt/hostedtoolcache/go/1.21.0/x64/src/database/sql/sql.go:2973 +0x2a8
  database/sql.(*Rows).initContextClose.func1()
      /opt/hostedtoolcache/go/1.21.0/x64/src/database/sql/sql.go:2949 +0x8f

...

Previous write at 0x00c0065901e0 by goroutine 115714:
  github.com/gliderlabs/ssh.(*sshContext).SetValue()
      /home/runner/go/pkg/mod/github.com/gliderlabs/[email protected]/context.go:123 +0x7d
...

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 9, 2023

IMO the complete fix could be like this:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants