Skip to content

Commit

Permalink
correct signer/apk2 data races
Browse files Browse the repository at this point in the history
This change fixes the obvious data races in the signer/apki2 tests. It
also adds the `EMPTY_TESTCACHE` environment variable to
run-unit-tests.sh to allow easy resetting of the Go test cache to
reproduce data races easily.

When run with `docker run unit-test -e RACE_TEST=1`, the following data
race was found:

```
==================
WARNING: DATA RACE
Read at 0x000000988860 by goroutine 21:
  github.com/mozilla-services/autograph/signer/apk2.TestNewSigner.func1()
      /app/src/autograph/signer/apk2/apk2_test.go:39 +0x40
  testing.tRunner()
      /usr/lib/go-1.22/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/lib/go-1.22/src/testing/testing.go:1742 +0x40

Previous write at 0x000000988860 by goroutine 22:
  github.com/mozilla-services/autograph/signer/apk2.TestNewSigner.func2()
      /app/src/autograph/signer/apk2/apk2_test.go:48 +0x3c
  testing.tRunner()
      /usr/lib/go-1.22/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/lib/go-1.22/src/testing/testing.go:1742 +0x40

Goroutine 21 (running) created at:
  testing.(*T).Run()
      /usr/lib/go-1.22/src/testing/testing.go:1742 +0x5e4
  github.com/mozilla-services/autograph/signer/apk2.TestNewSigner()
      /app/src/autograph/signer/apk2/apk2_test.go:36 +0x44
  testing.tRunner()
      /usr/lib/go-1.22/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/lib/go-1.22/src/testing/testing.go:1742 +0x40

Goroutine 22 (running) created at:
  testing.(*T).Run()
      /usr/lib/go-1.22/src/testing/testing.go:1742 +0x5e4
  github.com/mozilla-services/autograph/signer/apk2.TestNewSigner()
      /app/src/autograph/signer/apk2/apk2_test.go:45 +0x60
  testing.tRunner()
      /usr/lib/go-1.22/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/lib/go-1.22/src/testing/testing.go:1742 +0x40
==================
```

This was caused by tests modifying two global variables `apk2signerconf`
and `apk2signerconfModeV3enabled`. This was worsened by the tests being
marked as `t.Parallel` allowing them to be run concurrently.

This change removes those globals and provides their data from two new
functions in the tests, instead. The names are still bad in some cases,
but we're fixing problems, not trying to clean up the world.

Updates AUT-188
  • Loading branch information
jmhodges committed Aug 6, 2024
1 parent d6da10e commit 443eadb
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 18 deletions.
5 changes: 5 additions & 0 deletions bin/run_unit_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ make test
# run monitor unit tests
make -C tools/autograph-monitor test

EMPTY_TESTCACHE=${EMPTY_TESTCACHE:-""}
if [ "$EMPTY_TESTCACHE" = "1" ]; then
go clean -testcache
fi

if [ "$RACE_TEST" = "1" ]; then
make race
make -C tools/autograph-monitor race
Expand Down
45 changes: 27 additions & 18 deletions signer/apk2/apk2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestNewSigner(t *testing.T) {
t.Run("valid no mode", func(t *testing.T) {
t.Parallel()

s := assertNewSignerWithConfOK(t, apk2signerconf)
s := assertNewSignerWithConfOK(t, newApk2SignerConf())
if s.v3Enabled {
t.Fatalf("%s: unexpectedly enabled v3 signing", t.Name())
}
Expand All @@ -45,8 +45,9 @@ func TestNewSigner(t *testing.T) {
t.Run("valid empty mode", func(t *testing.T) {
t.Parallel()

apk2signerconf.Mode = ""
s := assertNewSignerWithConfOK(t, apk2signerconf)
missingMode := newApk2SignerConf()
missingMode.Mode = ""
s := assertNewSignerWithConfOK(t, missingMode)
if s.v3Enabled {
t.Fatalf("%s: unexpectedly enabled v3 signing", t.Name())
}
Expand All @@ -55,7 +56,7 @@ func TestNewSigner(t *testing.T) {
t.Run("valid v3enabled mode", func(t *testing.T) {
t.Parallel()

s := assertNewSignerWithConfOK(t, apk2signerconfModeV3enabled)
s := assertNewSignerWithConfOK(t, newApk2SignerConfWithModeV3())
if !s.v3Enabled {
t.Fatalf("%s: did not enable v3 signing", t.Name())
}
Expand All @@ -64,39 +65,39 @@ func TestNewSigner(t *testing.T) {
t.Run("invalid type", func(t *testing.T) {
t.Parallel()

invalidConf := apk2signerconf
invalidConf := newApk2SignerConf()
invalidConf.Type = "badType"
assertNewSignerWithConfErrs(t, invalidConf)
})

t.Run("invalid ID", func(t *testing.T) {
t.Parallel()

invalidConf := apk2signerconf
invalidConf := newApk2SignerConf()
invalidConf.ID = ""
assertNewSignerWithConfErrs(t, invalidConf)
})

t.Run("invalid Mode", func(t *testing.T) {
t.Parallel()

invalidConf := apk2signerconf
invalidConf := newApk2SignerConf()
invalidConf.Mode = "invalidEnabled"
assertNewSignerWithConfErrs(t, invalidConf)
})

t.Run("invalid PrivateKey", func(t *testing.T) {
t.Parallel()

invalidConf := apk2signerconf
invalidConf := newApk2SignerConf()
invalidConf.PrivateKey = ""
assertNewSignerWithConfErrs(t, invalidConf)
})

t.Run("invalid Certificate", func(t *testing.T) {
t.Parallel()

invalidConf := apk2signerconf
invalidConf := newApk2SignerConf()
invalidConf.Certificate = ""
assertNewSignerWithConfErrs(t, invalidConf)
})
Expand All @@ -105,6 +106,8 @@ func TestNewSigner(t *testing.T) {
func TestConfig(t *testing.T) {
t.Parallel()

apk2Conf := newApk2SignerConf()
mode3EnabledConf := newApk2SignerConfWithModeV3()
type fields struct {
Configuration signer.Configuration
}
Expand All @@ -116,16 +119,16 @@ func TestConfig(t *testing.T) {
{
name: "config without mode",
fields: fields{
Configuration: apk2signerconf,
Configuration: apk2Conf,
},
want: apk2signerconf,
want: apk2Conf,
},
{
name: "config v3enabled mode",
fields: fields{
Configuration: apk2signerconfModeV3enabled,
Configuration: mode3EnabledConf,
},
want: apk2signerconfModeV3enabled,
want: mode3EnabledConf,
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -155,7 +158,7 @@ func TestConfig(t *testing.T) {
func TestOptionsAreEmpty(t *testing.T) {
t.Parallel()

s := assertNewSignerWithConfOK(t, apk2signerconf)
s := assertNewSignerWithConfOK(t, newApk2SignerConf())
defaultOpts := s.GetDefaultOptions()
expectedOpts := Options{}
if defaultOpts != expectedOpts {
Expand All @@ -165,7 +168,7 @@ func TestOptionsAreEmpty(t *testing.T) {

func TestSignFile(t *testing.T) {
// initialize a signer
s := assertNewSignerWithConfOK(t, apk2signerconf)
s := assertNewSignerWithConfOK(t, newApk2SignerConf())

// sign input data
signedFile, err := s.SignFile(testAPK, s.GetDefaultOptions())
Expand Down Expand Up @@ -301,17 +304,23 @@ i+UaOY4AHXEAn1t3FuRW4J2W4tku4XmAZys9ATX0/LVbm/R3pqGYmTAqv0SDnStM
Gg/He+3S+8Rq0zqXAbOVJDVSTCRV5C9ZOmTWedBzaqmykScsCxLSpmEffy2RrtBU
dNKAPtSx4o34NaTpxg==
-----END CERTIFICATE-----`
apk2signerconf = signer.Configuration{
)

func newApk2SignerConf() signer.Configuration {
return signer.Configuration{
ID: "apk2test",
Type: Type,
PrivateKey: apk2TestPrivateKeyPEM,
Certificate: apk2TestCert,
}
apk2signerconfModeV3enabled = signer.Configuration{
}

func newApk2SignerConfWithModeV3() signer.Configuration {
return signer.Configuration{
ID: "apk2testv3enabled",
Type: Type,
Mode: "v3enabled",
PrivateKey: apk2TestPrivateKeyPEM,
Certificate: apk2TestCert,
}
)
}

0 comments on commit 443eadb

Please sign in to comment.