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

FuzzParseDN test was failed on GitHub Actions #472

Closed
t2y opened this issue Nov 16, 2023 · 9 comments
Closed

FuzzParseDN test was failed on GitHub Actions #472

t2y opened this issue Nov 16, 2023 · 9 comments

Comments

@t2y
Copy link
Contributor

t2y commented Nov 16, 2023

I encountered some failures with the result of a job on GitHub Actions.

I will investigate why this error happens on GitHub Actions.

WARNING: DATA RACE
Write at 0x000000c3c628 by goroutine 38:
  github.com/go-ldap/ldap.FuzzParseDN()
      /home/runner/work/ldap/ldap/fuzz_test.go:14 +0x32
  testing.fRunner()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/fuzz.go:700 +0x168
  testing.runFuzzTests.func1()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/fuzz.go:5[20](https://github.com/go-ldap/ldap/actions/runs/6781841980/job/18432888679#step:5:21) +0x47

Previous read at 0x000000c3c628 by goroutine 76:
  github.com/go-asn1-ber/asn1-ber.readPacket()
      /home/runner/go/pkg/mod/github.com/go-asn1-ber/[email protected]/ber.go:352 +0x31b
  github.com/go-asn1-ber/asn1-ber.readPacket()
      /home/runner/go/pkg/mod/github.com/go-asn1-ber/[email protected]/ber.go:326 +0x1079
  github.com/go-asn1-ber/asn1-ber.ReadPacket()
      /home/runner/go/pkg/mod/github.com/go-asn1-ber/[email protected]/ber.go:[21](https://github.com/go-ldap/ldap/actions/runs/6781841980/job/18432888679#step:5:22)8 +0x2e4
  github.com/go-ldap/ldap.(*Conn).reader()
      /home/runner/work/ldap/ldap/conn.go:598 +0x2be
  github.com/go-ldap/ldap.(*Conn).Start.func1()
      /home/runner/work/ldap/ldap/conn.go:267 +0x39

Goroutine 38 (running) created at:
  testing.runFuzzTests()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/fuzz.go:520 +0xdde
  testing.(*M).Run()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1720 +0xaf2
  main.main()
      _testmain.go:259 +0x3a9

Goroutine 76 (finished) created at:
  github.com/go-ldap/ldap.(*Conn).Start()
      /home/runner/work/ldap/ldap/conn.go:267 +0x9d
  github.com/go-ldap/ldap.DialURL()
      /home/runner/work/ldap/ldap/conn.go:[24](https://github.com/go-ldap/ldap/actions/runs/6781841980/job/18432888679#step:5:25)6 +0x587
  github.com/go-ldap/ldap.TestSearchAsyncAndCancel()
      /home/runner/work/ldap/ldap/ldap_test.go:[37](https://github.com/go-ldap/ldap/actions/runs/6781841980/job/18432888679#step:5:38)8 +0x55
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:14[39](https://github.com/go-ldap/ldap/actions/runs/6781841980/job/18432888679#step:5:40) +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1486 +0x[47](https://github.com/go-ldap/ldap/actions/runs/6781841980/job/18432888679#step:5:48)
@t2y
Copy link
Contributor Author

t2y commented Nov 16, 2023

Rarely, but I could reproduce it in my local environment. It's a concurrent race condition issue, I guess.

@t2y
Copy link
Contributor Author

t2y commented Nov 19, 2023

The job is here.

    - name: Build and Validate
      run: |
        cd ${{ matrix.branch }}
        go vet .
        go test .
        go test -cover -race -cpu 1,2,4 .
        go build .

@t2y
Copy link
Contributor Author

t2y commented Nov 19, 2023

I confirmed this CLI would fail.

$ go test -cover -race -cpu 1,2,4 .
==================
WARNING: DATA RACE
Write at 0x000000d016a0 by goroutine 1003:
  github.com/go-ldap/ldap.FuzzParseDN()
...

@t2y
Copy link
Contributor Author

t2y commented Nov 20, 2023

I understood this configuration is module global variable and multiple goroutines access to the variable. In FuzzParseDN(), the variable changed, so it occurred DATA RACE.

ber.MaxPacketLengthBytes = 65536

@t2y
Copy link
Contributor Author

t2y commented Nov 20, 2023

Fuzz test has two modes. So FuzzParseDN() is always called with seed corpus.

There are two modes of running your fuzz test: as a unit test (default go test), or with fuzzing (go test -fuzz=FuzzTestName).

Fuzz tests are run much like a unit test by default. Each seed corpus entry will be tested against the fuzz target, reporting any failures before exiting.

https://go.dev/security/fuzz/

@t2y
Copy link
Contributor Author

t2y commented Nov 20, 2023

There are different requirements for each test.

  • unit test mode: should not change ber.MaxPacketLengthBytes
  • fuzzing mode: should limit ber.MaxPacketLengthBytes

@t2y
Copy link
Contributor Author

t2y commented Nov 20, 2023

Setting a value before all tests by TestMain resolves it. ber.MaxPacketLengthBytes limit is not affected for other tests.

func TestMain(m *testing.M) {
	// For fuzz tests
	// See https://github.com/go-asn1-ber/asn1-ber/blob/04301b4b1c5ff66221f8f8a394f814a9917d678a/fuzz_test.go#L33-L37
	// for why this limitation is necessary
	ber.MaxPacketLengthBytes = 65536

	code := m.Run()
	os.Exit(code)
}

@t2y
Copy link
Contributor Author

t2y commented Nov 20, 2023

I don't know why v3 directory doesn't have fuzz_test.go.

t2y added a commit to t2y/ldap that referenced this issue Nov 20, 2023
… of changing ber's module global variable for fuzz tests go-ldap#472
johnweldon pushed a commit that referenced this issue Nov 21, 2023
… of changing ber's module global variable for fuzz tests #472 (#473)
@johnweldon
Copy link
Member

I don't know why v3 directory doesn't have fuzz_test.go.

Yes, we should make sure the v3 directory matches the root (until we develop a proper branch strategy)

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

No branches or pull requests

3 participants