Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: events leak that can occur if allocation fails #2771

Merged

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Oct 9, 2024

rare in practice, found by fuzzing


This change is Reviewable

@Green-Sky Green-Sky force-pushed the fix_event_leak_on_alloc_fail branch from d0cd28b to 1fabce5 Compare October 9, 2024 10:51
@Green-Sky Green-Sky added this to the v0.2.20 milestone Oct 9, 2024
@Green-Sky Green-Sky added bug Bug fix for the user, not a fix to a build script P3 Low priority labels Oct 9, 2024
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 40 of 40 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@Green-Sky
Copy link
Member Author

Not sure how we should fix the mallocfail test. the send message autotest times out, be cause the event fails to allocate and thus gets dropped.

@nurupo
Copy link
Member

nurupo commented Oct 20, 2024

Well, the allocation failure is propagated to the test code via tox_events_add()'s return value, so the test could notice the failure and change its behavior accordingly. Simply changing the test to succeed and terminate on the first allocation failure it encounters seems wrong though -- the test tests sending messages, no message got sent, yet the test succeeded. It clearly should still fail in this case. But as far as testing with mallocfail, we want the test to succeed and we want to test the entire code with mallocfail, but just exit the test with success on the first allocation failure. The obvious way to achieve all of this that comes to mind is changing all the tests to keep re-trying to allocate on all possible allocation failures as mallocfail does succeed eventually. Have the retries be limited to some number though, just to avoid infinite loops in case it never succeeds due to some bug or something. Might also want to have that retrying behavior under a flag that is turned on only when testing using mallocfail, as without mallocfail we would want the tests to fail right away and not waste the time retrying to allocate.

@nurupo
Copy link
Member

nurupo commented Oct 20, 2024

Actually, scratch that. Reading mallocfail's README, the intended use is to not modify the code to accommodate the use of mallocfail, but to keep re-running it with mallocfail many times until no new entries are added to mallocfail's hashes file, at which point all mallocs will succeed and the test should succeed too, keeping note of unusual terminations/crashes along the way.

@Green-Sky Green-Sky force-pushed the fix_event_leak_on_alloc_fail branch from 1fabce5 to 15ca352 Compare November 8, 2024 11:19
@Green-Sky
Copy link
Member Author

for reference #2783 was a fix specifically for the ci fail.

rare in practice, found by fuzzing
@Green-Sky Green-Sky force-pushed the fix_event_leak_on_alloc_fail branch from 15ca352 to 4067628 Compare November 8, 2024 13:52
@toktok-releaser toktok-releaser merged commit 4067628 into TokTok:master Nov 8, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fix for the user, not a fix to a build script P3 Low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants