*: 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 <james@tailscale.com>

.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
+    - name: checkout
-      id: build
+      uses: actions/checkout@v3
-      uses: google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@master
+    - name: Restore Build Cache
-      # continue-on-error makes steps.build.conclusion be 'success' even if
+      uses: actions/cache@v3
-      # steps.build.outcome is 'failure'. This means this step does not
-      # contribute to the job's overall pass/fail evaluation.
-      continue-on-error: true
       with:
-       oss-fuzz-project-name: 'tailscale'
+        # Note: unlike the other setups, this is only grabbing the mod download
-       dry-run: false
+        # cache, rather than the whole mod directory, as the download cache
-       language: go
+        # contains zips that can be unpacked in parallel faster than they can be
-    - name: report unexpectedly broken fuzz build
+        # fetched and extracted by tar
-      if: steps.build.outcome == 'failure' && env.TS_FUZZ_CURRENTLY_BROKEN != 'true'
+        path: |
-      run: |
+          ~/.cache/go-build
-        echo "fuzzer build failed, see above for why"
+          ~/go/pkg/mod/cache
-        echo "if the failure is due to OSS-Fuzz not being on the latest Go yet,"
+          ~\AppData\Local\go-build
-        echo "set TS_FUZZ_CURRENTLY_BROKEN=true in .github/workflows/test.yml"
+        # The -2- here should be incremented when the scheme of data to be
-        echo "to temporarily disable fuzzing until OSS-Fuzz works again."
+        # cached changes (e.g. path above changes).
-        exit 1
+        key: ${{ github.job }}-${{ runner.os }}-go-2-${{ hashFiles('**/go.sum') }}
-    - name: report unexpectedly working fuzz build
+        restore-keys: |
-      if: steps.build.outcome == 'success' && env.TS_FUZZ_CURRENTLY_BROKEN == 'true'
+          ${{ github.job }}-${{ runner.os }}-go-2-
-      run: |
+    - name: Restore fuzz corpus cache
-        echo "fuzzer build succeeded, but we expect it to be broken"
+      uses: actions/cache@v3
-        echo "please set TS_FUZZ_CURRENTLY_BROKEN=false in .github/workflows/test.yml"
+      with:
-        echo "to reenable fuzz testing"
+        path: |
-        exit 1
+          ~/.cache/go-build/fuzz
-    - name: run fuzzers
+        # 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 the fuzzers whenever they're able to build, even if we're going to
+      run: ./tool/go test -fuzz=${{ matrix.test }} -fuzztime=60s ${{ matrix.pkg }}
-      # 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
-      with:
-        oss-fuzz-project-name: 'tailscale'
-        fuzz-seconds: 300
-        dry-run: false
-        language: go
     - 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
+        name: testdata
-        path: ./out/artifacts
+        path: ./**/testdata
 
   depaware:
     runs-on: ubuntu-22.04

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)
+		}
+	})
+}

disco/disco_fuzzer.go

@@ -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
-}

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)
+	})
+}

net/stun/stun_fuzzer.go

@@ -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
-}