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

Revert "don't cache more than 1024 entries, to avoid DoS attacks" #101

Merged
merged 1 commit into from
Jul 6, 2021

Conversation

grandcat
Copy link
Owner

@grandcat grandcat commented Jul 6, 2021

Reverts #94

Causes 2/2 test runs to fail (race) or even crash on master.
@marten-seemann fyi

@marten-seemann
Copy link
Contributor

That's a manifestation of #98, namely the fact that with the current API, there's no way to tell when the mainloop go routine has shut down.

#100 should fix that race condition by reworking the client-side API.

What the test does and what causes the race condition is reduce this value, to speed up the test:

zeroconf/client.go

Lines 29 to 30 in 60421fc

// DoS protection: we won't cache more than 1024 entries when receiving entries.
var maxSentEntries = 1024

Once the test is done, it resets it to the old value. Problem is, as mainloop exits eventually after the context is canceled, it may still read from that variable.

@grandcat
Copy link
Owner Author

grandcat commented Jul 6, 2021

Thanks for the explanation.
In any case, it increases changes for failing tests on master. Therefore, I'd prefer reverting this Pr until we have a more stabilized version, may it be a change in client.go, may it be a slightly different test case.

@grandcat grandcat merged commit 60fce55 into master Jul 6, 2021
@marten-seemann
Copy link
Contributor

I don't think there's a lot we can do to fix the test case, other than adding ridiculous long sleeps. Is there any chance we can get #100 merged any time soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants