[Libwebsockets] #331 client application crashes libwebsocket_context_destroy on flaky network tests

Andy Green andy at warmcat.com
Tue Oct 20 00:27:25 CEST 2015



On 20 October 2015 01:24:38 GMT+08:00, Vikas Gahlan <vikas.gahlan at chargepoint.com> wrote:
>Excellent. Thanks Andy. New fix works for me. We can close this ticket.
>Any proposed date for next release v1.5 ?  

I'm just waiting a bit for problems to come to light from the last batch of commits.

But usually even if you wait a good while, a lot of people only try it after you tag out a new version... so this week I guess.

-Andy

>Vikas Gahlan
>
>-----Original Message-----
>From: Andy Green [mailto:andy.green at linaro.org] On Behalf Of Andy Green
>Sent: Monday, October 19, 2015 10:05 PM
>To: Vikas Gahlan; libwebsockets at ml.libwebsockets.org
>Subject: RE: [Libwebsockets] #331 client application crashes
>libwebsocket_context_destroy on flaky network tests
>
>
>
>On 19 October 2015 23:42:23 GMT+08:00, Vikas Gahlan
><vikas.gahlan at chargepoint.com> wrote:
>>Initially, I also thought about same fix what you proposed but when I 
>>looked into lws_change_pollfd() function code, there are more error 
>>handling in this function where full cleanup using
>>libwebsocket_close_and_free_session() not required IMO. As I am new to
>
>The full close processing for a wsi is very adaptive to the wsi state,
>but he does assume the wsi has an entry in the fd table.  Basically if
>he got added to the fd table, he must get removed from it when
>destroyed and the close flow will handle that.
>
>As you suspect later, lws_change_pollfd() has no meaning if the wsi not
>in the fd table (since it's not part of the poll() then, and he has no
>pollfd entry to manipulate).  So that should never happen.
>
>>libwebsocket and still reviewing code to understand architecture , so
>I 
>>put changes in oom:  handling to ensure
>>libwebsocket_close_and_free_session() is invoked only when there is 
>>valid entry in fdtable.
>
>Well... if it got as far as putting a socket fd into the fd table, and
>then errored out, he needs to do the full close processing.  So that
>fix does fix something that needs fixing.
>
>If there are other things that need fixing, we can solve that as well
>separately.
>
>>I am using multithreaded application in which
>>libwebsocket_client_connect_2 is invoked in 2 different context at 
>>context setup:
>>1) When my client application invokes libwebsocket_client_connect() 
>>after successful context creation using libwebsocket_create_context().
>>2) If libwebsocket_client_connect() is successful in #A which is
>mostly 
>>successful then app creates specific service thread for handling 
>>websocket communication.
>>
>>If you are sure that lws_change_pollfd() is invoked only when sock is 
>>added in fdlist, then your fix of changes oom to failed in case of
>>lws_change_pollfd() error is fine. 
>
>User code can end up calling that asynchronously when using the 'call
>me back when wsi writeable' callbacks, but he won't / can't / shouldn't
>do so until the wsi has transitioned to ws protocol which is a long
>time after he's in the fd list.
>
>HTTP processing that uses pollfd manipulation likewise doesn't happen
>until we did a poll() and the socket said he had the initial http
>headers for us; that only happens after he's added to the fd table.
>
>No library use of apis changing POLL state should try to operate on a
>socket fd not in the table, since it won't work anyway.
>
>-Andy
>
>>Vikas Gahlan
>>
>>-----Original Message-----
>>From: Andy Green [mailto:andy.green at linaro.org] On Behalf Of Andy
>Green
>>Sent: Sunday, October 18, 2015 4:25 PM
>>To: Vikas Gahlan; libwebsockets at ml.libwebsockets.org
>>Subject: Re: [Libwebsockets] #331 client application crashes in 
>>libwebsocket_context_destroy on flaky network tests
>>
>>
>>
>>On 18 October 2015 18:39:39 GMT+08:00, Andy Green <andy at warmcat.com>
>>wrote:
>>>
>>>
>>>On 18 October 2015 14:19:20 GMT+08:00, Vikas Gahlan 
>>><vikas.gahlan at chargepoint.com> wrote:
>>>>Attached patch work for me.
>>>>
>>>>Here is exact trace flow of the crash:
>>>>
>>>>libwebsocket_client_connect_2
>>>>libwebsocket_client_connect_2: address mywebsocket.com closed 
>>>>libwebsocket_context_destroy
>>>>close: just_kill_connection
>>>>remove_wsi_socket_from_fds: wsi=0x7a0d8, sock=9, fds pos=1
>>>>
>>>>
>>>>below fix works for me:
>>>>
>>>>From 300e50b2fe6a033a109d5f1f15208cb8490363e4 Mon Sep 17 00:00:00
>>2001
>>>>From: vgahlan-cpi <vikas.gahlan at chargepoint.com>
>>>>Date: Sat, 17 Oct 2015 22:40:23 -0700
>>>>Subject: [PATCH] fdpolllist cleanup requires if wsi socket exists in
>
>>>>fdtable to avoid double free in libwebsocket_context_destroy
>>>>
>>>>---
>>>>lib/client-handshake.c | 9 ++++++---
>>>>1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>>diff --git a/lib/client-handshake.c b/lib/client-handshake.c index
>>>>7293a41..d7d72e6 100644
>>>>--- a/lib/client-handshake.c
>>>>+++ b/lib/client-handshake.c
>>>>@@ -291,9 +291,12 @@ struct libwebsocket 
>>>>*libwebsocket_client_connect_2(
>>>>        return wsi;
>>>>
>>>>oom4:
>>>>-       lws_free(wsi->u.hdr.ah);
>>>>-       lws_free(wsi);
>>>>-       return NULL;
>>>>+        /*In case wsi added successfully in fd polllist, clean-up
>>>>requires for fdtable and session data properly using 
>>>>libwebsocket_close_and_free_session().*/
>>>>+        if(wsi->position_in_fds_table < 0  || wsi->sock < 0 ){
>>>
>>>Thanks... this definitely is a real problem, but the fix needs to be
>a
>>
>>>bit closer to the source of the problem I think.
>>>
>>>I'll push an alternative fix for you to try in the next minutes.
>>
>>How about this?  He was the only path to oom4 after insertion into the
>
>>fds list.
>>
>>http://git.libwebsockets.org/cgi-bin/cgit/libwebsockets/commit/?id=b5cf
>>69fdb57c4025572cde1007e0ff16108d4f80
>>
>>-Andy
>>
>>>-Andy
>>>
>>>>+                lws_free(wsi->u.hdr.ah);
>>>>+                lws_free(wsi);
>>>>+                return NULL;
>>>>+        }
>>>>
>>>>failed:
>>>>        libwebsocket_close_and_free_session(context, wsi,
>>>>--
>>>>1.8.3.1
>>>>
>>>>Vikas Gahlan
>>>>ChargePoint | chargepoint.com
>>>>Ofiice: +91-124-4889450 Ext. 464 | Mobile +91-9873232008
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>This email and any attachments are intended for the sole use of the 
>>>>intended recipient(s) and
>>>>contain(s) confidential information that may be proprietary,
>>>privileged
>>>>or copyrighted under
>>>>applicable law. If you are not the intended recipient, do not read, 
>>>>copy, or forward this email message or any attachments and delete
>>this
>>>>email message and any attachments immediately.
>>>>
>>>>---------------------------------------------------------------------
>>>>-
>>>>--
>>>>
>>>>_______________________________________________
>>>>Libwebsockets mailing list
>>>>Libwebsockets at ml.libwebsockets.org
>>>>http://ml.libwebsockets.org/mailman/listinfo/libwebsockets
>>>
>>>_______________________________________________
>>>Libwebsockets mailing list
>>>Libwebsockets at ml.libwebsockets.org
>>>http://ml.libwebsockets.org/mailman/listinfo/libwebsockets
>>
>>
>>
>>
>>
>>
>>
>>This email and any attachments are intended for the sole use of the 
>>intended recipient(s) and
>>contain(s) confidential information that may be proprietary,
>privileged 
>>or copyrighted under applicable law. If you are not the intended 
>>recipient, do not read, copy, or forward this email message or any 
>>attachments and delete this email message and any attachments 
>>immediately.
>
>
>
>
>
>
>
>This email and any attachments are intended for the sole use of the
>intended recipient(s) and
>contain(s) confidential information that may be proprietary, privileged
>or copyrighted under
>applicable law. If you are not the intended recipient, do not read,
>copy, or forward this email
>message or any attachments and delete this email message and any
>attachments immediately.




More information about the Libwebsockets mailing list