[PATCH] make ssl acceptor not exit frequently for timeout

Hi,

I guess the timeout argument is given to ssl:transport_accept by mistake,
so I make a patch to prevent ssl acceptor exiting frequently for timeout.

This patch is made against yaws-1.77 but also can be applied to yaws-1.79
successfully.

From 92ce9b3d9a46bc6b30108992795bfc067dde3263 Mon Sep 17 00:00:00 2001
From: Liu Yubao <yubao.liu <at> gmail.com>
Date: Sat, 31 Jan 2009 23:17:29 +0800
Subject: [PATCH] make ssl acceptor not exit frequently for timeout

---
 src/yaws_server.erl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/yaws_server.erl b/src/yaws_server.erl
index 80e939d..1baebde 100644
--- a/src/yaws_server.erl
+++ b/src/yaws_server.erl
 <at>  <at>  -730,7 +730,7  <at>  <at>  do_accept(GS) when GS#gs.ssl == nossl ->
     ?Debug("wait in accept ... ~n",[]),
     gen_tcp:accept(GS#gs.l);
 do_accept(GS) when GS#gs.ssl == ssl ->
-    ssl:transport_accept(GS#gs.l,10000).
+    ssl:transport_accept(GS#gs.l).

 

[PATCH] don't erase whole process dictionary if don't have initial backup

Hi,

Erasing whole process dictionary when get(init_db) returns
undefined will make the clause below in proc_lib:exit_p/2
throw a badmatch exception:
    %% erl5.6.5/lib/stdlib-1.15.5/src/proc_lib.erl:158
    exit_p(Class, Reason) ->
        {M,F,A} = get('$initial_call'),

The yaws-1.79 has fixed this problem in another way, but I
think my patch(made against yaws-1.77) is more proper because:

*  put(init_db, I) is only called in aloop();
*  returning undefined from get(init_db) means we haven't enter
   aloop(), so we shouldn't call clear() always.

Best regards,
Liu Yubao

From dedaa03e951e155e261a909e8cde7e88d38374ff Mon Sep 17 00:00:00 2001
From: Liu Yubao <yubao.liu <at> gmail.com>
Date: Sun, 1 Feb 2009 03:16:45 +0800
Subject: [PATCH] don't erase whole process dictionary if don't have initial backup

Erasing whole process dictionary when get(init_db) returns
undefined will make the clause below in proc_lib:exit_p/2
throw a badmatch exception:
    %% erl5.6.5/lib/stdlib-1.15.5/src/proc_lib.erl:158
    exit_p(Class, Reason) ->

Re: [PATCH] make ssl acceptor not exit frequently for timeout

Liu Yubao wrote:
> Hi,
> 
> I guess the timeout argument is given to ssl:transport_accept by mistake,
> so I make a patch to prevent ssl acceptor exiting frequently for timeout.
> 
> This patch is made against yaws-1.77 but also can be applied to yaws-1.79
> successfully.
> 

Looking at the code, I agree it doesn't look good. But I honestly don't
think it's a mistake. There are quite a few high profile sites that run
an awful lot of ssl traffic.

However, I cannot remember why that timeout clause is there.
Anyone ?? Tobbe, Magnus ??

/klacke

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com

Re: [PATCH] don't erase whole process dictionary if don't have initial backup

Liu Yubao wrote:
> Hi,
> 
> Erasing whole process dictionary when get(init_db) returns
> undefined will make the clause below in proc_lib:exit_p/2
> throw a badmatch exception:
>     %% erl5.6.5/lib/stdlib-1.15.5/src/proc_lib.erl:158
>     exit_p(Class, Reason) ->
>         {M,F,A} = get('$initial_call'),
> 
> The yaws-1.79 has fixed this problem in another way, but I
> think my patch(made against yaws-1.77) is more proper because:
> 
> *  put(init_db, I) is only called in aloop();
> *  returning undefined from get(init_db) means we haven't enter
>    aloop(), so we shouldn't call clear() always.

Agree, Thanks, Applied.

/klacke

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com

Re: [PATCH] make ssl acceptor not exit frequently for timeout

Claes Wikstrom wrote:
> Liu Yubao wrote:
>> Hi,
>>
>> I guess the timeout argument is given to ssl:transport_accept by mistake,
>> so I make a patch to prevent ssl acceptor exiting frequently for timeout.
>>
>> This patch is made against yaws-1.77 but also can be applied to yaws-1.79
>> successfully.
>>
> 
> Looking at the code, I agree it doesn't look good. But I honestly don't
> think it's a mistake. There are quite a few high profile sites that run
> an awful lot of ssl traffic.
I think it's at least a security vulnerability, compared to the first
yaws_server:do_accept/1, it's reasonable to hang forever to wait a new
tcp connection but not reasonable to hang forever to finish a ssl handshake.

On the other side, I'm afraid the current style will harm the performance
to accept new ssl connection.

> 
> However, I cannot remember why that timeout clause is there.
> Anyone ?? Tobbe, Magnus ??
> 
> /klacke
> 

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)

Re: [PATCH] make ssl acceptor not exit frequently for timeout

Hi,

AFAIK there is due to a problem in the SSL implementation. If you don't
time out on accept, the accept loop can hang forever after a while [1].

After looking at esock_ssl alot (due to other issues I think I've posted
about here) and having issues with zombie ssl connections in Ejabberd
I've realised that the current ssl implementation is... kinda broken.

[1] http://www.erlang.org/pipermail/erlang-questions/2004-June/012691.html

Claes Wikstrom wrote:
> Liu Yubao wrote:
>   
>> Hi,
>>
>> I guess the timeout argument is given to ssl:transport_accept by mistake,
>> so I make a patch to prevent ssl acceptor exiting frequently for timeout.
>>
>> This patch is made against yaws-1.77 but also can be applied to yaws-1.79
>> successfully.
>>
>>     
>
> Looking at the code, I agree it doesn't look good. But I honestly don't
> think it's a mistake. There are quite a few high profile sites that run
> an awful lot of ssl traffic.
>
> However, I cannot remember why that timeout clause is there.
> Anyone ?? Tobbe, Magnus ??

Re: [PATCH] make ssl acceptor not exit frequently for timeout

Oscar Hellström wrote:
> Hi,
> 
> AFAIK there is due to a problem in the SSL implementation. If you don't
> time out on accept, the accept loop can hang forever after a while [1].
> 
> After looking at esock_ssl alot (due to other issues I think I've posted
> about here) and having issues with zombie ssl connections in Ejabberd
> I've realised that the current ssl implementation is... kinda broken.
> 
> [1] http://www.erlang.org/pipermail/erlang-questions/2004-June/012691.html

But the proper fix to that problem was the introduction of
transport_accept() - right. I do remember that prior to
the existence of ssl:transport_accept() we had some gross hacks in
Yaws whereby some 10,20 processes we used to accept()
simultaneously.

So, my question still stands, does anyone have a proper clue as
why the timeout is there. Kreditor folks, you ought to be
very interested in this.

/klacke

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com

Re: [PATCH] make ssl acceptor not exit frequently for timeout

Liu Yubao wrote:
> 
> I think it's at least a security vulnerability, compared to the first
> yaws_server:do_accept/1, it's reasonable to hang forever to wait a new
> tcp connection but not reasonable to hang forever to finish a ssl handshake.
> 

Long time ago OTP had a function called ssl:accept(), that function
did the actual accept() and also the ssl server side handshake. That
was bad and all sorts of bad implications.

The transport_accept() is merely the actual accepting, no ssl at all.
So, there are as far as I can tell no security, nor any DOS
implications.

> On the other side, I'm afraid the current style will harm the performance
> to accept new ssl connection.
> 

Agreed, as well as possibly unnecessary and ugly  and weird.

>> However, I cannot remember why that timeout clause is there.
>> Anyone ?? Tobbe, Magnus ??
>>

Tobbe, Magnus ?

/klacke

------------------------------------------------------------------------------

Re: [PATCH] don't erase whole process dictionary if don't have initial backup

Claes Wikstrom wrote:
> Liu Yubao wrote:
>> Hi,
>>
>> Erasing whole process dictionary when get(init_db) returns
>> undefined will make the clause below in proc_lib:exit_p/2
>> throw a badmatch exception:
>>     %% erl5.6.5/lib/stdlib-1.15.5/src/proc_lib.erl:158
>>     exit_p(Class, Reason) ->
>>         {M,F,A} = get('$initial_call'),
>>
>> The yaws-1.79 has fixed this problem in another way, but I
>> think my patch(made against yaws-1.77) is more proper because:
>>
>> *  put(init_db, I) is only called in aloop();
>> *  returning undefined from get(init_db) means we haven't enter
>>    aloop(), so we shouldn't call clear() always.
> 
> 
> Agree, Thanks, Applied.
> 
Aha, you combined my patch without reverting the original fix...
I think it's better to remove the "put(init_db, I)" clause because:

* The original fix isn't required any more to make proc_lib:exit_p/2 work;
* The current *combined* patch does some useless work;
    erase_transients():
         put(init_db, I),
         lists:foreach(......),
    aloop() calls init_db():    

Re: [PATCH] don't erase whole process dictionary if don't have initial backup

Liu Yubao wrote:

>>
> Aha, you combined my patch without reverting the original fix...
> I think it's better to remove the "put(init_db, I)" clause because:
>

Yes, you are right. Thanks

/klacke

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com