redsocks icon indicating copy to clipboard operation
redsocks copied to clipboard

Make sure to run `accept_enable` when handling `accept_backoff_ev`

Open agusdallalba opened this issue 8 years ago • 2 comments

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.

agusdallalba avatar Mar 11 '17 19:03 agusdallalba

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.

darkk avatar Mar 15 '17 13:03 darkk

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.

agusdallalba avatar Mar 15 '17 14:03 agusdallalba