diff --git a/mock/mock.go b/mock/mock.go index a13c37f3b..b22f979bc 100644 --- a/mock/mock.go +++ b/mock/mock.go @@ -977,7 +977,7 @@ func (args Arguments) Diff(objects []interface{}) (string, int) { actualFmt = "(Missing)" } else { actual = objects[i] - actualFmt = fmt.Sprintf("(%[1]T=%[1]v)", actual) + actualFmt = safeFormatArg(actual) } if len(args) <= i { @@ -985,7 +985,7 @@ func (args Arguments) Diff(objects []interface{}) (string, int) { expectedFmt = "(Missing)" } else { expected = args[i] - expectedFmt = fmt.Sprintf("(%[1]T=%[1]v)", expected) + expectedFmt = safeFormatArg(expected) } if matcher, ok := expected.(argumentMatcher); ok { @@ -1310,3 +1310,27 @@ func isFuncSame(f1, f2 *runtime.Func) bool { return f1File == f2File && f1Loc == f2Loc } + +// safeFormatArg formats an argument for display in diff output, handling +// concurrent modification safely. For types that contain inherent references +// (pointers, maps, slices, channels), fmt.Sprintf("%v") would traverse the +// underlying data structure, which races with concurrent writers. For maps +// this is a non-recoverable fatal error; for pointers and slices it triggers +// data races detectable by -race. +// +// To avoid this, reference types are formatted with %p (address only) instead +// of %v. All other types (structs, primitives, strings) are value types that +// cannot race, so they use the full %v representation. +// +// See https://github.com/stretchr/testify/issues/1597 +func safeFormatArg(v interface{}) string { + if v == nil { + return fmt.Sprintf("(%[1]T=%[1]v)", v) + } + switch reflect.TypeOf(v).Kind() { + case reflect.Ptr, reflect.Map, reflect.Slice, reflect.Chan: + return fmt.Sprintf("(%T=%p)", v, v) + default: + return fmt.Sprintf("(%[1]T=%[1]v)", v) + } +} diff --git a/mock/mock_test.go b/mock/mock_test.go index 3dc9e0b1e..66aebfc3c 100644 --- a/mock/mock_test.go +++ b/mock/mock_test.go @@ -2044,6 +2044,81 @@ func Test_Arguments_Diff_WithArgMatcher(t *testing.T) { assert.Contains(t, diff, `No differences.`) } +// Test_Arguments_Diff_ConcurrentPointerModification verifies that +// Arguments.Diff does not race when a pointer argument is concurrently +// modified. This is a regression test for https://github.com/stretchr/testify/issues/1597. +// Adapted from https://github.com/stretchr/testify/pull/1598. +func Test_Arguments_Diff_ConcurrentPointerModification(t *testing.T) { + t.Parallel() + + type data struct { + Value string + } + + arg := &data{Value: "original"} + args := Arguments([]interface{}{Anything}) + + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < 1000; i++ { + arg.Value = fmt.Sprintf("modified-%d", i) + } + }() + + // Without the fix, this races with the goroutine above because + // fmt.Sprintf("%v") traverses *data while the goroutine writes to it. + for i := 0; i < 100; i++ { + args.Diff([]interface{}{arg}) + } + <-done +} + +// Test_Arguments_Diff_ConcurrentMapModification verifies that Arguments.Diff +// does not race when a map argument is concurrently modified. +// Raised by @brackendawson in https://github.com/stretchr/testify/pull/1598#discussion_r1869482623. +func Test_Arguments_Diff_ConcurrentMapModification(t *testing.T) { + t.Parallel() + + arg := map[string]string{"key": "original"} + args := Arguments([]interface{}{Anything}) + + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < 1000; i++ { + arg["key"] = fmt.Sprintf("modified-%d", i) + } + }() + + for i := 0; i < 100; i++ { + args.Diff([]interface{}{arg}) + } + <-done +} + +// Test_Arguments_Diff_ConcurrentSliceModification verifies that Arguments.Diff +// does not race when a slice argument is concurrently modified. +func Test_Arguments_Diff_ConcurrentSliceModification(t *testing.T) { + t.Parallel() + + arg := []string{"original"} + args := Arguments([]interface{}{Anything}) + + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < 1000; i++ { + arg[0] = fmt.Sprintf("modified-%d", i) + } + }() + + for i := 0; i < 100; i++ { + args.Diff([]interface{}{arg}) + } + <-done +} + func Test_Arguments_Assert(t *testing.T) { t.Parallel() @@ -2271,7 +2346,7 @@ func TestArgumentMatcherToPrintMismatchWithReferenceType(t *testing.T) { defer func() { if r := recover(); r != nil { matchingExp := regexp.MustCompile( - `\s+mock: Unexpected Method Call\s+-*\s+GetTimes\(\[\]int\)\s+0: \[\]int\{1\}\s+The closest call I have is:\s+GetTimes\(mock.argumentMatcher\)\s+0: mock.argumentMatcher\{.*?\}\s+Diff:.*\(\[\]int=\[1\]\) not matched by func\(\[\]int\) bool\nat: \[[^\]]+mock\/mock_test.go`) + `\s+mock: Unexpected Method Call\s+-*\s+GetTimes\(\[\]int\)\s+0: \[\]int\{1\}\s+The closest call I have is:\s+GetTimes\(mock.argumentMatcher\)\s+0: mock.argumentMatcher\{.*?\}\s+Diff:.*\(\[\]int=0x[0-9a-f]+\) not matched by func\(\[\]int\) bool\nat: \[[^\]]+mock\/mock_test.go`) assert.Regexp(t, matchingExp, r) } }() @@ -2306,7 +2381,7 @@ func TestClosestCallFavorsFirstMock(t *testing.T) { defer func() { if r := recover(); r != nil { - diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -2,4 \+2,4 @@\s+\(bool\) true,\s+- \(bool\) true,\s+- \(bool\) true\s+\+ \(bool\) false,\s+\+ \(bool\) false\s+}\s+Diff: 0: FAIL: \(\[\]bool=\[(true\s?|false\s?){3}]\) != \(\[\]bool=\[(true\s?|false\s?){3}\]\)` + diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -2,4 \+2,4 @@\s+\(bool\) true,\s+- \(bool\) true,\s+- \(bool\) true\s+\+ \(bool\) false,\s+\+ \(bool\) false\s+}\s+Diff: 0: FAIL: \(\[\]bool=0x[0-9a-f]+\) != \(\[\]bool=0x[0-9a-f]+\)` matchingExp := regexp.MustCompile(unexpectedCallRegex(`TheExampleMethod7([]bool)`, `0: \[\]bool{true, false, false}`, `0: \[\]bool{true, true, true}`, diffRegExp)) assert.Regexp(t, matchingExp, r) } @@ -2324,7 +2399,7 @@ func TestClosestCallUsesRepeatabilityToFindClosest(t *testing.T) { defer func() { if r := recover(); r != nil { - diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -1,4 \+1,4 @@\s+\(\[\]bool\) \(len=3\) {\s+- \(bool\) false,\s+- \(bool\) false,\s+\+ \(bool\) true,\s+\+ \(bool\) true,\s+\(bool\) false\s+Diff: 0: FAIL: \(\[\]bool=\[(true\s?|false\s?){3}]\) != \(\[\]bool=\[(true\s?|false\s?){3}\]\)` + diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -1,4 \+1,4 @@\s+\(\[\]bool\) \(len=3\) {\s+- \(bool\) false,\s+- \(bool\) false,\s+\+ \(bool\) true,\s+\+ \(bool\) true,\s+\(bool\) false\s+Diff: 0: FAIL: \(\[\]bool=0x[0-9a-f]+\) != \(\[\]bool=0x[0-9a-f]+\)` matchingExp := regexp.MustCompile(unexpectedCallRegex(`TheExampleMethod7([]bool)`, `0: \[\]bool{true, true, false}`, `0: \[\]bool{false, false, false}`, diffRegExp)) assert.Regexp(t, matchingExp, r) }