Make the test element type notice self move assignment - #73
Merged
Conversation
Counter::Obj's move assignment is a plain field copy, so moving an object onto itself preserves its value. That made #65 and #66 invisible: both were pure data corruption, the fuzz harness generates exactly the inputs that trigger them and compares against std::vector after every operation, and it still saw nothing wrong. Abort instead. A container moving an element onto itself means it passed an overlapping range to std::move or std::move_backward, which neither allows, so there is no legitimate case to preserve. Writing x = std::move(x) by hand is fine and this check would be wrong on a general purpose type, but Obj exists to be put into containers. With the two fixes reverted, insert_input_iterator now aborts where it used to pass, and the fuzz corpus fails on its first of 1651 entries. Not gated on COUNTER_ENABLE_UNORDERED_SET like the other invariant checks here: that macro guards two hash lookups per call, this is one pointer comparison and is worth having unconditionally. Closes #67 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #67.
Counter::Obj::operator=(Obj&&)is a plain field copy, so moving an object onto itself preserves its value. That is why #65 and #66 survived: both were pure data corruption,test/fuzz/api.cppgenerates exactly the inputs that trigger them, and it compares againststd::vectorafter every operation — and still saw nothing.Proof it works
With both fixes reverted:
insert_input_iteratoris a pre-existing test that passed while the bug was live. The fuzz corpus fails on its first of 1651 entries.With the fixes in place the whole suite is clean, so no legitimate operation in the library self-move-assigns.
Why this and not a std::string fuzz lane
The issue suggested either. Detecting it in the element type turns out to be strictly stronger:
std::stringlane depends on implementation-defined behaviour.[container.reqmts]only promises "valid but unspecified" after a self-move; libstdc++ happening to leave the string empty is not a guarantee. A future stdlib or a different SSO threshold could silently restore the blind spot.repeat_oneofbranches tear down and rebuild the containers withassertEqcommented out.It is also a 4-line change rather than duplicating the whole harness, every branch of which is wired to
Counter::Objbookkeeping (c.set(counts),counts.check_all_done()) with nostd::stringanalogue.Two deliberate deviations from the file's convention
The other invariant checks here sit inside
#if COUNTER_ENABLE_UNORDERED_SET. This one does not: that macro guards two hash-set lookups per call, while this is a single pointer comparison worth having unconditionally.Counter::Objis not used by any benchmark, so nothing measured is affected.The message carries
"self move assignment"rather than the bare__func__the other sites use, because__func__isoperator=for both the copy and move overloads.Known remaining gap
This catches self-move only. A double-move (same source moved into two destinations) would still pass, because
Obj's move leaves the source untouched, so the duplicate looks valid. Marking the source moved-from and rejecting a second move would cover it — worth a follow-up, not done here.