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

Race condition in SOCKS5 test #276

Closed
fortuna opened this issue Sep 6, 2024 · 4 comments
Closed

Race condition in SOCKS5 test #276

fortuna opened this issue Sep 6, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@fortuna
Copy link
Contributor

fortuna commented Sep 6, 2024

It seems TestConnectWithoutAuth has a race condition, caught in https://github.com/Jigsaw-Code/outline-sdk/actions/runs/10733360009/job/29766592024?pr=272.

Test output:

==================
WARNING: DATA RACE
Read at 0x00c000084ee3 by goroutine 84:
  testing.(*common).logDepth()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1011 +0xcd
  testing.(*common).log()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1004 +0x7d
  testing.(*common).Log()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1045 +0x55
  github.com/Jigsaw-Code/outline-sdk/transport/socks5.TestConnectWithoutAuth.func1()
      D:/a/outline-sdk/outline-sdk/transport/socks5/stream_dialer_test.go:180 +0xa4

Previous write at 0x00c000084ee3 by goroutine 83:
  testing.tRunner.func1()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1582 +0x8fa
  runtime.deferreturn()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/runtime/panic.go:477 +0x30
  testing.(*T).Run.func1()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1648 +0x44

Goroutine 84 (running) created at:
  github.com/Jigsaw-Code/outline-sdk/transport/socks5.TestConnectWithoutAuth()
      D:/a/outline-sdk/outline-sdk/transport/socks5/stream_dialer_test.go:178 +0x22d
  testing.tRunner()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1648 +0x44

Goroutine 83 (running) created at:
  testing.(*T).Run()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1648 +0x845
  testing.runTests.func1()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1595 +0x261
  testing.runTests()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:20[52](https://github.com/Jigsaw-Code/outline-sdk/actions/runs/10733360009/job/29766592024?pr=272#step:11:53) +0x8ad
  testing.(*M).Run()
      C:/hostedtoolcache/windows/go/1.21.13/x64/src/testing/testing.go:1925 +0xcd7
  main.main()
      _testmain.go:79 +0x2bd
==================
FAIL
exit status 1
FAIL	github.com/Jigsaw-Code/outline-sdk/transport/socks5	0.060s
goos: windows
goarch: amd64

@amircybersec Can you take a look?

@amircybersec
Copy link
Contributor

@fortuna This issue has been addressed in the main code. We are getting this error because fortuna-tc branch is not on the HEAD of main branch and does not have the latest changes to Socks5 transport code (including UPD changes).

This is the latest code:
https://github.com/Jigsaw-Code/outline-sdk/blob/main/transport/socks5/stream_dialer_test.go

@fortuna
Copy link
Contributor Author

fortuna commented Sep 6, 2024

The error started after I merged the latest main into my branch. Did I do it wrong?

@amircybersec
Copy link
Contributor

amircybersec commented Sep 6, 2024

@fortuna thanks for the feedback.

I believe if I remove this line, it would resolve the issue:

t.Log("server is listening...")

in the following go routine:

	go func() {
		err := server.Serve(listener)
		t.Log("server is listening...")
		if !errors.Is(err, net.ErrClosed) && err != nil {
			require.NoError(t, err) // Assert no error if it's not the expected close error
		}
	}()

https://github.com/Jigsaw-Code/outline-sdk/blob/37cc3c5ab8b75f14461b45dad0e41101f7e6462d/transport/socks5/stream_dialer_test.go#L180C3-L180C34

Since I am accessing Testing.t within the go routine, this object might be written to by around test and is probably causing this race condition.

Should I do this fix on a new branch? or would you like to apply it to fortuna-tc first?

@sbruens sbruens added the bug Something isn't working label Sep 10, 2024
@amircybersec
Copy link
Contributor

issue was addressed in #278

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants