fix: prevent deadlock when argument's String() calls MethodCalled#1876
Open
ChihebBENCHEIKH1 wants to merge 1 commit intostretchr:masterfrom
Open
fix: prevent deadlock when argument's String() calls MethodCalled#1876ChihebBENCHEIKH1 wants to merge 1 commit intostretchr:masterfrom
ChihebBENCHEIKH1 wants to merge 1 commit intostretchr:masterfrom
Conversation
When Mock.MethodCalled is called with an argument that implements Stringer, and that String() method calls MethodCalled (e.g. a mock passed as its own argument), a deadlock occurs because Diff uses %v to format arguments while the mutex is held. This adds an unexported matchCount method that checks argument equality without formatting strings, and uses it in findExpectedCall and findClosestCall instead of Diff. The Diff formatting now happens after the mutex is released. All existing tests pass with zero regressions. Fixes stretchr#1719
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1719.
When
Mock.MethodCalledis called with an argument that implementsStringer, and thatString()method callsMethodCalled(e.g. a mock passed as its own argument), a deadlock occurs. This happens becausefindExpectedCallcallsArguments.Diffwhich uses%vto format arguments — triggeringString()— while the mutex is already held.Root cause
Fix
Added an unexported
matchCountmethod that checks argument equality without formatting strings, avoiding any calls to user-definedString()orGoString()methods. Used it in:findExpectedCall— the main matching path (hot path under mutex)findClosestCall— the error path (also under mutex)Diffformatting now happens after the mutex is released inMethodCalled, so it's safe for arguments to call back into the mock.Test
Added
TestIssue1719StringerDeadlockwith a 5-second timeout that reproduces the exact scenario from the issue (mock passed as its own argument whereString()callsMethodCalled).Results
All existing tests pass with zero regressions: