From 6c992d3e6019813986e52d60b1c1b3aff07b27a4 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Fri, 14 Apr 2023 20:36:36 -0700 Subject: [PATCH] *: replace ossfuzz with go native fuzz invocations The fuzzing actions are the longest actions in our CI right now. This would be a good and happy thing if the bulk of the time was spent fuzzing, but sadly the bulk of the time is spent doing build preparations due to the nature of the ossfuzz docker setup. Only two out of our 5 packages that contain fuzzers were ossfuzz fuzzers, the rest are Go 1.18+ fuzzers. These two are converted to go fuzzers, and then the github workflow is updated to just go test fuzz. The action setup contains two separate steps for actions-cache, one that handles the fuzzing corpus specifically so that we shuttle forward any interesting corpus over time. If the action fails, it will upload all the testdata/* directories, which will include the data necessary to commit interesting cases for permanent redistribution. Signed-off-by: James Tucker --- .github/workflows/test.yml | 112 ++++++++++++++++--------------------- disco/disco_fuzz_test.go | 95 +++++++++++++++++++++++++++++++ disco/disco_fuzzer.go | 17 ------ net/stun/stun_fuzz_test.go | 14 +++++ net/stun/stun_fuzzer.go | 12 ---- 5 files changed, 157 insertions(+), 93 deletions(-) create mode 100644 disco/disco_fuzz_test.go delete mode 100644 disco/disco_fuzzer.go create mode 100644 net/stun/stun_fuzz_test.go delete mode 100644 net/stun/stun_fuzzer.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 60d2066de..e8f663745 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -2,20 +2,6 @@ # both PRs and merged commits, and for the latter reports failures to slack. name: CI -env: - # Our fuzz job, powered by OSS-Fuzz, fails periodically because we upgrade to - # new Go versions very eagerly. OSS-Fuzz is a little more conservative, and - # ends up being unable to compile our code. - # - # When this happens, we want to disable the fuzz target until OSS-Fuzz catches - # up. However, we also don't want to forget to turn it back on when OSS-Fuzz - # can once again build our code. - # - # This variable toggles the fuzz job between two modes: - # - false: we expect fuzzing to be happy, and should report failure if it's not. - # - true: we expect fuzzing is broken, and should report failure if it start working. - TS_FUZZ_CURRENTLY_BROKEN: false - on: push: branches: @@ -296,63 +282,61 @@ jobs: fuzz: - # This target periodically breaks (see TS_FUZZ_CURRENTLY_BROKEN at the top - # of the file), so it's more complex than usual: the 'build fuzzers' step - # might fail, and depending on the value of 'TS_FUZZ_CURRENTLY_BROKEN', that - # might or might not be fine. The steps after the build figure out whether - # the success/failure is expected, and appropriately pass/fail the job - # overall accordingly. - # - # Practically, this means that all steps after 'build fuzzers' must have an - # explicit 'if' condition, because the default condition for steps is - # 'success()', meaning "only run this if no previous steps failed". - if: github.event_name == 'pull_request' runs-on: ubuntu-22.04 + strategy: + matrix: + include: + - pkg: ./disco + test: FuzzDisco + - pkg: ./net/dns/resolver + test: FuzzClampEDNSSize + - pkg: ./net/stun + test: FuzzStun + - pkg: ./util/deephash + test: FuzzTime + - pkg: ./util/deephash + test: FuzzAddr + - pkg: ./util/hashx + test: Fuzz + steps: - - name: build fuzzers - id: build - uses: google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@master - # continue-on-error makes steps.build.conclusion be 'success' even if - # steps.build.outcome is 'failure'. This means this step does not - # contribute to the job's overall pass/fail evaluation. - continue-on-error: true + - name: checkout + uses: actions/checkout@v3 + - name: Restore Build Cache + uses: actions/cache@v3 with: - oss-fuzz-project-name: 'tailscale' - dry-run: false - language: go - - name: report unexpectedly broken fuzz build - if: steps.build.outcome == 'failure' && env.TS_FUZZ_CURRENTLY_BROKEN != 'true' - run: | - echo "fuzzer build failed, see above for why" - echo "if the failure is due to OSS-Fuzz not being on the latest Go yet," - echo "set TS_FUZZ_CURRENTLY_BROKEN=true in .github/workflows/test.yml" - echo "to temporarily disable fuzzing until OSS-Fuzz works again." - exit 1 - - name: report unexpectedly working fuzz build - if: steps.build.outcome == 'success' && env.TS_FUZZ_CURRENTLY_BROKEN == 'true' - run: | - echo "fuzzer build succeeded, but we expect it to be broken" - echo "please set TS_FUZZ_CURRENTLY_BROKEN=false in .github/workflows/test.yml" - echo "to reenable fuzz testing" - exit 1 - - name: run fuzzers - id: run - # Run the fuzzers whenever they're able to build, even if we're going to - # report a failure because TS_FUZZ_CURRENTLY_BROKEN is set to the wrong - # value. - if: steps.build.outcome == 'success' - uses: google/oss-fuzz/infra/cifuzz/actions/run_fuzzers@master + # Note: unlike the other setups, this is only grabbing the mod download + # cache, rather than the whole mod directory, as the download cache + # contains zips that can be unpacked in parallel faster than they can be + # fetched and extracted by tar + path: | + ~/.cache/go-build + ~/go/pkg/mod/cache + ~\AppData\Local\go-build + # The -2- here should be incremented when the scheme of data to be + # cached changes (e.g. path above changes). + key: ${{ github.job }}-${{ runner.os }}-go-2-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ github.job }}-${{ runner.os }}-go-2- + - name: Restore fuzz corpus cache + uses: actions/cache@v3 with: - oss-fuzz-project-name: 'tailscale' - fuzz-seconds: 300 - dry-run: false - language: go + path: | + ~/.cache/go-build/fuzz + # Uses an incrementing key to constantly collide and upload, but this + # cache action is kept separate from any build cache action. + key: fuzz-${{ matrix.pkg }}-${{ matrix.test }}-${{ github.run_id }} + restore-keys: | + fuzz-${{ matrix.pkg }}-${{ matrix.test }}- + - name: fuzz ${{matrix.pkg}} ${{matrix.test}} + id: run + run: ./tool/go test -fuzz=${{ matrix.test }} -fuzztime=60s ${{ matrix.pkg }} - name: upload crash uses: actions/upload-artifact@v3 - if: steps.run.outcome != 'success' && steps.build.outcome == 'success' + if: steps.run.outcome != 'success' with: - name: artifacts - path: ./out/artifacts + name: testdata + path: ./**/testdata depaware: runs-on: ubuntu-22.04 diff --git a/disco/disco_fuzz_test.go b/disco/disco_fuzz_test.go new file mode 100644 index 000000000..105897fa3 --- /dev/null +++ b/disco/disco_fuzz_test.go @@ -0,0 +1,95 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package disco + +import ( + "bytes" + "reflect" + "testing" + + "golang.org/x/exp/slices" +) + +func FuzzDisco(f *testing.F) { + f.Fuzz(func(t *testing.T, data1 []byte) { + if data1 == nil { + return + } + data2 := make([]byte, 0, len(data1)) + + m1, e1 := Parse(data1) + if m1 == nil || reflect.ValueOf(m1).IsNil() { + if e1 == nil { + t.Fatal("nil message and nil error!") + } + t.Logf("message result is actually nil, can't be serialized again") + return + } + + data2 = m1.AppendMarshal(data2) + m2, e2 := Parse(data2) + if m2 == nil || reflect.ValueOf(m2).IsNil() { + if e2 == nil { + t.Fatal("nil message and nil error!") + } + t.Errorf("second message result is actually nil!") + } + + t.Logf("m1: %#v", m1) + t.Logf("m2: %#v", m1) + t.Logf("data1:\n%x", data1) + t.Logf("data2:\n%x", data2) + + if e1 != nil && e2 != nil { + if e1.Error() != e2.Error() { + t.Errorf("error mismatch: %v != %v", e1, e2) + } + return + } + + // Explicitly ignore the case where the fuzzer made a different version + // byte, it's not interesting. + data1[1] = v0 + // The protocol doesn't have a length at this layer, and so it will + // ignore meaningless trailing data such as a key that is more than 0 + // bytes, but less than keylen bytes. + if len(data2) < len(data1) { + data1 = data1[:len(data2)] + } + + if !bytes.Equal(data1, data2) { + t.Errorf("data mismatch:\n%x\n%x", data1, data2) + } + + switch t1 := m1.(type) { + case *Ping: + t2, ok := m2.(*Ping) + if !ok { + t.Errorf("m1 and m2 are not the same type") + } + if *t1 != *t2 { + t.Errorf("m1 and m2 are not the same") + } + case *Pong: + t2, ok := m2.(*Pong) + if !ok { + t.Errorf("m1 and m2 are not the same type") + } + if *t1 != *t2 { + t.Errorf("m1 and m2 are not the same") + } + case *CallMeMaybe: + t2, ok := m2.(*CallMeMaybe) + if !ok { + t.Errorf("m1 and m2 are not the same type") + } + + if !slices.Equal(t1.MyNumber, t2.MyNumber) { + t.Errorf("m1 and m2 are not the same") + } + default: + t.Fatalf("unknown message type %T", m1) + } + }) +} diff --git a/disco/disco_fuzzer.go b/disco/disco_fuzzer.go deleted file mode 100644 index b9ffabfb0..000000000 --- a/disco/disco_fuzzer.go +++ /dev/null @@ -1,17 +0,0 @@ -// Copyright (c) Tailscale Inc & AUTHORS -// SPDX-License-Identifier: BSD-3-Clause -//go:build gofuzz - -package disco - -func Fuzz(data []byte) int { - m, _ := Parse(data) - - newBytes := m.AppendMarshal(data) - parsedMarshall, _ := Parse(newBytes) - - if m != parsedMarshall { - panic("Parsing error") - } - return 1 -} diff --git a/net/stun/stun_fuzz_test.go b/net/stun/stun_fuzz_test.go new file mode 100644 index 000000000..1daf03521 --- /dev/null +++ b/net/stun/stun_fuzz_test.go @@ -0,0 +1,14 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package stun + +import "testing" + +func FuzzStun(f *testing.F) { + f.Fuzz(func(t *testing.T, data []byte) { + _, _, _ = ParseResponse(data) + + _, _ = ParseBindingRequest(data) + }) +} diff --git a/net/stun/stun_fuzzer.go b/net/stun/stun_fuzzer.go deleted file mode 100644 index 6f0c9e3b0..000000000 --- a/net/stun/stun_fuzzer.go +++ /dev/null @@ -1,12 +0,0 @@ -// Copyright (c) Tailscale Inc & AUTHORS -// SPDX-License-Identifier: BSD-3-Clause -//go:build gofuzz - -package stun - -func FuzzStunParser(data []byte) int { - _, _, _ = ParseResponse(data) - - _, _ = ParseBindingRequest(data) - return 1 -}