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

Make sure to run accept_enable when handling accept_backoff_ev #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

agusdallalba
Copy link

This fixes issue #99.

There's a situation where the connection pressure is solved but redsocks doesn't resume listening to new connections.

  1. redsocks_conn_max is hit, conn_pressure runs.
  2. There are no long-idle connections, so conn_pressure adds the accept_backoff_ev event and disables all listeners.
  3. Some time passes.
  4. accept_backoff_ev gets triggered and redsocks_accept_backoff gets called.
  5. conn_pressure_ongoing() == true, so conn_pressure runs.
  6. This time there's a connection that's more than 7440s old. conn_pressure drops it and returns.
  7. Now conn_pressure_ongoing() == false, but accept_enable isn't called because the conditional was evaluated before running conn_pressure.
  8. redsocks_accept_backoff returns, so accept_backoff_ev is not pending anymore and accept_enable will never be called.

@darkk
Copy link
Owner

darkk commented Mar 15, 2017

Thank you for the test-case!
The fix seems a bit questionable to me - enabling listeners on every exponential backoff iteration also looks like invariant violation, but the bug is there for sure.

@agusdallalba
Copy link
Author

I could have added an accept_enable() in redsocks.c:1032, right before the return, but my reasoning was that the accept_backoff_ev event being pending "means" that redsocks isn't listening at the moment—and the rest of the code treats it that way. For example conn_pressure_lowered calls accept_enable() only if this event is pending.

So, in this way, event_add(&accept_backoff_ev, &tvdelay) semantically is 'stop listening for tvdelay'. If the event handler can return without re-enabling the listeners this meaning is lost. I chose to call accept_enable() as soon as possible within the event handler to prevent another bug like this one from ever happening.

@darkk darkk force-pushed the master branch 6 times, most recently from 7fa694d to 5df6a30 Compare February 1, 2018 20:41
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