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.

Sign up for GitHub

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

Jump to bottom

Race condition in tsh play #11479

Closed
zmb3 opened this issue Mar 25, 2022 · 1 comment · Fixed by #11491
Closed

Race condition in tsh play #11479

zmb3 opened this issue Mar 25, 2022 · 1 comment · Fixed by #11491
Assignees
@zmb3
Labels
bug tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Comments

@zmb3
Copy link
Collaborator

zmb3 commented Mar 25, 2022 •
edited
Loading

I was looking at the code in lib/client/player.go and suspected that we may have a data race, since waitUntil() is just polling a variable that's updated by another goroutine without any synchronization.

I built tsh with the race detector enabled and tried to playback a session, and sure enough, the race detector tripped.

WARNING: DATA RACE
Write at 0x00c00023d910 by goroutine 56:
  github.com/gravitational/teleport/lib/client.(*sessionPlayer).Stop()
      /Users/zmb/teleport/lib/client/player.go:69 +0x98
  github.com/gravitational/teleport/lib/client.(*sessionPlayer).playRange.func2()
      /Users/zmb/teleport/lib/client/player.go:218 +0x5fc

Previous read at 0x00c00023d910 by main goroutine:
  github.com/gravitational/teleport/lib/client.playSession()
      /Users/zmb/teleport/lib/client/api.go:3578 +0x530
  github.com/gravitational/teleport/lib/client.(*TeleportClient).Play()
      /Users/zmb/teleport/lib/client/api.go:1658 +0x43c
  main.onPlay()
      /Users/zmb/teleport/tool/tsh/tsh.go:822 +0x650
  main.Run()
      /Users/zmb/teleport/tool/tsh/tsh.go:712 +0x1a8dc
  main.main()
      /Users/zmb/teleport/tool/tsh/tsh.go:337 +0x2dc

Goroutine 56 (running) created at:
  github.com/gravitational/teleport/lib/client.(*sessionPlayer).playRange()
      /Users/zmb/teleport/lib/client/player.go:183 +0x268
  github.com/gravitational/teleport/lib/client.(*sessionPlayer).Play()
      /Users/zmb/teleport/lib/client/player.go:61 +0x528
  github.com/gravitational/teleport/lib/client.playSession()
      /Users/zmb/teleport/lib/client/api.go:3575 +0x518
  github.com/gravitational/teleport/lib/client.(*TeleportClient).Play()
      /Users/zmb/teleport/lib/client/api.go:1658 +0x43c
  main.onPlay()
      /Users/zmb/teleport/tool/tsh/tsh.go:822 +0x650
  main.Run()
      /Users/zmb/teleport/tool/tsh/tsh.go:712 +0x1a8dc
  main.main()
      /Users/zmb/teleport/tool/tsh/tsh.go:337 +0x2dc
==================
Found 1 data race(s)

This code is 5+ years old, so I suspect it has always been an issue and we just have no test coverage for SSH session playback.
@zmb3 zmb3 added bug tsh tsh - Teleport's command line tool for logging into nodes running Teleport. good-starter-issue Good starter issue to start contributing to Teleport labels Mar 25, 2022
@zmb3
Copy link
Collaborator Author

zmb3 commented Mar 25, 2022

If somebody picks this up - you may find the code in lib/web/desktop/player.go helpful. It implements a similar pattern, but uses a condvar to detect concurrent state changes rather than the racy polling approach used for tsh playback.

zmb3 added a commit that referenced this issue Mar 27, 2022
@zmb3
Fix tsh player issues
dd258b2 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
@zmb3 zmb3 mentioned this issue Mar 27, 2022
Merged
@zmb3 zmb3 removed the good-starter-issue Good starter issue to start contributing to Teleport label Mar 28, 2022
zmb3 added a commit that referenced this issue Mar 29, 2022
@zmb3
Fix tsh player issues
eaabfea 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
@zmb3 zmb3 self-assigned this Mar 29, 2022
zmb3 added a commit that referenced this issue Mar 29, 2022
@zmb3
Fix tsh player issues (#11491)
8901b1c 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
@zmb3 zmb3 mentioned this issue Mar 29, 2022
Merged
@zmb3 zmb3 mentioned this issue Mar 29, 2022
Merged
@zmb3 zmb3 mentioned this issue Mar 29, 2022
Merged
zmb3 added a commit that referenced this issue Mar 29, 2022
@zmb3
Fix tsh player issues (#11491)
925a224 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Mar 29, 2022
@zmb3
Fix tsh player issues (#11491)
e35d690 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Mar 29, 2022
@zmb3
Fix tsh player issues (#11491)
789cb79 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Mar 31, 2022
@zmb3
Fix tsh player issues (#11491)
9a6e743 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Mar 31, 2022
@zmb3
Fix tsh player issues (#11491)
2410d54 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Apr 1, 2022
@zmb3
Fix tsh player issues (#11491)
9c99177 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Apr 1, 2022
@zmb3
Fix tsh player issues (#11491)
cdefcbd 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Apr 1, 2022
@zmb3
Fix tsh player issues (#11491)
42c3471 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Apr 1, 2022
@zmb3
Fix tsh player issues (#11491)
b13f74b 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Apr 1, 2022
@zmb3
Fix tsh player issues (#11491)
d36ae15 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
zmb3 added a commit that referenced this issue Apr 1, 2022
@zmb3
Fix tsh player issues (#11491)
cba383e 
This commit fixes race conditions in the tsh session player by using
a condition variable to detect state changes rather than unsafely
polling a variable that is written by a separate goroutine.

In addition, fix an off by one error when resuming playback
after pausing. The player's position variable has always stored
the index of the last succesfully played event, so when we resume
playback we should start at position+1 to not re-play the previous
event twice.

Fixes #11479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zmb3 zmb3

Labels
bug tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix tsh player issues gravitational/teleport
1 participant
@zmb3