search

go-ldap / ldap

Basic LDAP v3 functionality for the GO programming language.
Other
2.23k stars 352 forks source link

FuzzParseDN test was failed on GitHub Actions #472

Closed t2y closed 6 months ago

t2y commented 10 months ago

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/asn1-ber@v1.5.5/ber.go:352 +0x31b
  github.com/go-asn1-ber/asn1-ber.readPacket()
      /home/runner/go/pkg/mod/github.com/go-asn1-ber/asn1-ber@v1.5.5/ber.go:326 +0x1079
  github.com/go-asn1-ber/asn1-ber.ReadPacket()
      /home/runner/go/pkg/mod/github.com/go-asn1-ber/asn1-ber@v1.5.5/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 commented 10 months ago

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

t2y commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

There are different requirements for each test.

t2y commented 10 months ago

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 commented 10 months ago

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

johnweldon commented 10 months ago

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)