Skip to content

Commit

Permalink
Reduce request rate when out of fd only.
Browse files Browse the repository at this point in the history 
Fixes reduced request rate problem for ssl/tls connections.
  • Loading branch information
@mmzeeman
mmzeeman committed Sep 26, 2014
1 parent 7c1ff12 commit 2afa95c
Showing 1 changed file with 9 additions and 10 deletions.
19 changes: 9 additions & 10 deletions src/mochiweb_socket_server.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,17 +296,16 @@ handle_info(Msg, State) when ?is_old_state(State) ->
handle_info(Msg, upgrade_state(State));
handle_info({'EXIT', Pid, normal}, State) ->
{noreply, recycle_acceptor(Pid, State)};
handle_info({'EXIT', Pid, Reason},
State=#mochiweb_socket_server{acceptor_pool=Pool}) ->
case sets:is_element(Pid, Pool) of
true ->
%% If there was an unexpected error accepting, log and sleep.
error_logger:error_report({?MODULE, ?LINE,
{acceptor_error, Reason}}),
timer:sleep(100);
false ->
ok
handle_info({'EXIT', Pid, {error, emfile}}, State) ->
error_logger:error_msg("No more file descriptors, reducing request rate.~n"),
timer:sleep(100),
{noreply, recycle_acceptor(Pid, State)};
handle_info({'EXIT', Pid, Reason}, State=#mochiweb_socket_server{acceptor_pool=Pool}) ->
ErrorType = case sets:is_element(Pid, Pool) of
true -> acceptor_error;
false -> request_error
end,
error_logger:error_report({?MODULE, ?LINE, {ErrorType, Reason}}),

This comment has been minimized.

Sign in to view

Copy link
@etrepum

etrepum Oct 8, 2014

Contributor

This looks like it may open up a different DOS attack if an inefficient error_logger is used and the attacker finds a way to quickly create errors
{noreply, recycle_acceptor(Pid, State)};

% this is what release_handler needs to get a list of modules,
Expand Down

8 comments on commit 2afa95c

@mmzeeman
Copy link
Member Author

Choose a reason for hiding this comment

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

But that won't be a 10 request per second DOS.

@arjan
Copy link
Member

@arjan arjan commented on 2afa95c Oct 9, 2014

Choose a reason for hiding this comment

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

Or the process hits the message inbox limit?

@etrepum
Copy link
Contributor

@etrepum etrepum commented on 2afa95c Oct 9, 2014

Choose a reason for hiding this comment

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

Well that depends in how big the Reason is. To avoid introducing a new attack vector the request errors shouldn't be logged here.

@mmzeeman
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I'm more worried about not being warned about possible bugs because nothing is logged than about DOS attacks via de error_logger. You can easily drop "normal" error messages by whitelisting them btw.

Something similar happened to me before with mochiweb + webmachine. I found this out the hard way. Sometimes connections where mysteriously dropped without leaving any trace of what happened. Only the client side noticed it, nothing in the server side logs.

The thing was that our request process sometimes received a message (timeout) which was picked up during receiving of the next http request. Mochiweb just silently dropped the connection without leaving a message that it received something out of the ordinary.

Those kind of bugs are hard to catch, but usually very easy to prevent.

This kind of unexpected behaviour, like sleeping for 100 ms for any kind of accept error, leads to all kinds of weird and hard to debug software.

@etrepum
Copy link
Contributor

@etrepum etrepum commented on 2afa95c Oct 9, 2014

Choose a reason for hiding this comment

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

Introducing error logging by default for requests is up for discussion, but it should be done separately from this issue.

The way I see it is that you're fixing a problem in SSL, but potentially introducing a regression somewhere else. A change in semantics for non-SSL requests shouldn't be hidden away in a fix for SSL.

We handled logging of errors during the request with our own try/catch in the handler that allowed us to quickly decide what to do about it without escalating to any other processes unless it was necessary to do so.

@mmzeeman
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI. Looking in my logs I see that there are a two responses from accept which should not be handled as errors as they are right now. econnaborted and {tls_alert, _}.

Both all normal things and don't require error reporting and loggin, you can just accept again.

And you can always get eagain back from accept right?

@etrepum
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the exact semantics of eagain but that sounds correct to me.

@mworrell
Copy link
Member

Choose a reason for hiding this comment

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

About the error logging. We could either use a configurable error logging module or add gen_event event handling?

Please sign in to comment.