[PATCH 1/2] gatppp: Check ppp instance before unref it

[PATCH 1/2] gatppp: Check ppp instance before unref it

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

---
 gatchat/gatppp.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index 1d41ded..d9b1627 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -446,6 +446,9 @@ void g_at_ppp_unref(GAtPPP *ppp)
 {
 	gboolean is_zero;
 
+	if (ppp == NULL)
+		return;
+
 	is_zero = g_atomic_int_dec_and_test(&ppp->ref_count);
 
 	if (is_zero == FALSE)
-- 
1.6.3.3

[PATCH 2/2] gathdlc: Unset disconnect function in unref

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

If we destroy PPP instance from g_at_ppp_unref, we should not invoke
io disconnect function.
---
 gatchat/gathdlc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
index 08a1939..0186e46 100644
--- a/gatchat/gathdlc.c
+++ b/gatchat/gathdlc.c
@@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc)
 		hdlc->record_fd = -1;
 	}
 
+	g_at_io_set_disconnect_function(hdlc->io, NULL, NULL);
 	g_at_io_unref(hdlc->io);
 	hdlc->io = NULL;
 
-- 
1.6.3.3

RE: [PATCH 2/2] gathdlc: Unset disconnect function in unref

[Zhang, Zhenhua]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

Hi,

Zhenhua Zhang wrote:
> If we destroy PPP instance from g_at_ppp_unref, we should not invoke
> io disconnect function. ---
> gatchat/gathdlc.c |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
> index 08a1939..0186e46 100644
> --- a/gatchat/gathdlc.c
> +++ b/gatchat/gathdlc.c
> @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc)
> 	hdlc->record_fd = -1; }
> 
> +	g_at_io_set_disconnect_function(hdlc->io, NULL, NULL);
> 	g_at_io_unref(hdlc->io); hdlc->io = NULL;

Here the problem is that disconnect function could be invoked if remote IO is disconnected after our ppp instance is freed. So we should avoid to invoke io_disconnect() in below case.

Entering new phase: 5
PPP: lcp: pppcp_timeout: current state 5:STOPPING
PPP: lcp: pppcp_generate_event: current state 5:STOPPING
PPP: event: 5 (TO-), action: 803, new_state: 3 (STOPPED)
PPP: lcp: pppcp_this_layer_finished: current state 3:STOPPED
Entering new phase: 0
ofonod[3114]: src/emulator.c:ppp_disconnect() 
Server: > \r\nNO CARRIER\r\n
ofonod[3114]: Pcui:< \r\n^BOOT:20291346,0,0,0,20\r\n
Server: < AT+CFUN=0\r
ofonod[3114]: src/emulator.c:cfun_cb() set CFUN to 0
Server: > \r\nOK\r\n


(gdb) bt
#0  0x0806faf6 in pppcp_generate_event (data=0x95bfde0, event_type=DOWN, packet=0x0, len=0)
    at gatchat/ppp_cp.c:638
#1  0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349
#2  0x001ea504 in g_source_callback_unref (cb_data=0x95a8678)
    at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1077
#3  0x001eaace in g_source_destroy_internal (source=0x95a86b0, context=<value optimized out>, have_lock=1)
    at /build/buildd/glib2.0-2.22.3/glib/gmain.c:856
#4  0x001eaedb in g_main_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1985
#5  IA__g_main_context_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2513
#6  0x001ee730 in g_main_context_iterate (context=0x95a5f80, block=<value optimized out>, dispatch=1, 
    self=0x95a6c58) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2591
#7  0x001eeb9f in IA__g_main_loop_run (loop=0x95a5ef0) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2799
#8  0x08094548 in main (argc=1, argv=0xbffb8ba4) at src/main.c:227
(gdb) frame 1
#1  0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349
349		pppcp_signal_down(ppp->lcp);
(gdb) p ppp
$6 = <value optimized out>
(gdb) info local
No locals.
(gdb) 

> --
> 1.6.3.3
> 
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono



Regards,
Zhenhua

Re: [PATCH 1/2] gatppp: Check ppp instance before unref it

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

Hi Zhenhua,

> ---
>  gatchat/gatppp.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
> index 1d41ded..d9b1627 100644
> --- a/gatchat/gatppp.c
> +++ b/gatchat/gatppp.c
> @@ -446,6 +446,9 @@ void g_at_ppp_unref(GAtPPP *ppp)
>  {
>  	gboolean is_zero;
>  
> +	if (ppp == NULL)
> +		return;
> +
>  	is_zero = g_atomic_int_dec_and_test(&ppp->ref_count);
>  
>  	if (is_zero == FALSE)

since we have been safe-guarding this in other unref functions as well,
I also applied this patch.

In general the calling code is doing something wrong here if it tries to
unref a NULL pointer.

Regards

Marcel

Re: [PATCH 2/2] gathdlc: Unset disconnect function in unref

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

Hi Zhenhua,

> @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc)
>  		hdlc->record_fd = -1;
>  	}
>  
> +	g_at_io_set_disconnect_function(hdlc->io, NULL, NULL);

Since GAtHDLC does not set the disconnect function, this really belongs
in GAtPPP.

>  	g_at_io_unref(hdlc->io);
>  	hdlc->io = NULL;
>  

However, I really question why this patch is necessary.  The only way
the disconnect function is not reset today is if the GAtIO is refed and
g_at_chat_resume is not called.

We do call g_at_io_set_disconnect function in g_at_chat_resume.

Regards,
-Denis

Re: [PATCH 2/2] gathdlc: Unset disconnect function in unref

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

Hi Zhenhua,

> Here the problem is that disconnect function could be invoked if remote IO is disconnected after our ppp instance is freed. So we should avoid to invoke io_disconnect() in below case.
> 
> Entering new phase: 5
> PPP: lcp: pppcp_timeout: current state 5:STOPPING
> PPP: lcp: pppcp_generate_event: current state 5:STOPPING
> PPP: event: 5 (TO-), action: 803, new_state: 3 (STOPPED)
> PPP: lcp: pppcp_this_layer_finished: current state 3:STOPPED
> Entering new phase: 0
> ofonod[3114]: src/emulator.c:ppp_disconnect() 
> Server: > \r\nNO CARRIER\r\n
> ofonod[3114]: Pcui:< \r\n^BOOT:20291346,0,0,0,20\r\n
> Server: < AT+CFUN=0\r
> ofonod[3114]: src/emulator.c:cfun_cb() set CFUN to 0
> Server: > \r\nOK\r\n
> 
> 
> (gdb) bt
> #0  0x0806faf6 in pppcp_generate_event (data=0x95bfde0, event_type=DOWN, packet=0x0, len=0)
>     at gatchat/ppp_cp.c:638
> #1  0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349
> #2  0x001ea504 in g_source_callback_unref (cb_data=0x95a8678)
>     at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1077
> #3  0x001eaace in g_source_destroy_internal (source=0x95a86b0, context=<value optimized out>, have_lock=1)
>     at /build/buildd/glib2.0-2.22.3/glib/gmain.c:856
> #4  0x001eaedb in g_main_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1985
> #5  IA__g_main_context_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2513
> #6  0x001ee730 in g_main_context_iterate (context=0x95a5f80, block=<value optimized out>, dispatch=1, 
>     self=0x95a6c58) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2591
> #7  0x001eeb9f in IA__g_main_loop_run (loop=0x95a5ef0) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2799
> #8  0x08094548 in main (argc=1, argv=0xbffb8ba4) at src/main.c:227
> (gdb) frame 1
> #1  0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349
> 349		pppcp_signal_down(ppp->lcp);
> (gdb) p ppp
> $6 = <value optimized out>
> (gdb) info local
> No locals.
> (gdb) 

It seems the real culprit is that GAtServer is not restoring the
io_disconnect on the GAtIO object in g_at_server_resume.  Why are we not
seeing this crash in test-server?

Regards,
-Denis

RE: [PATCH 2/2] gathdlc: Unset disconnect function in unref

[Zhang, Zhenhua]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

Hi Denis,

Denis Kenzior wrote:
> Hi Zhenhua,
> 
>> @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc) 
>>  	hdlc->record_fd = -1; }
>> 
>> +	g_at_io_set_disconnect_function(hdlc->io, NULL, NULL);
> 
> Since GAtHDLC does not set the disconnect function, this really
> belongs in GAtPPP. 

How about unset io_disconnect() in GAtPPP? Since GAtPPP doesn't have suspend/resume, the io_disconnect always exist until ppp is dead.
 
>>  	g_at_io_unref(hdlc->io);
>>  	hdlc->io = NULL;
>> 
> 
> However, I really question why this patch is necessary.  The only way
> the disconnect function is not reset today is if the GAtIO is refed
> and g_at_chat_resume is not called.
> 
> We do call g_at_io_set_disconnect function in g_at_chat_resume.

Good catch. I fixed it in gatserver and will send a patch out soon. Yes, after reset the disconnect function, the problem is gone.
 
> Regards,
> -Denis



Regards,
Zhenhua