[PATCH] stk: Add null pointer check

[PATCH] stk: Add null pointer check

[Lasse Kunnasluoto]

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

Fixes a crash when SIM Session End is received while waiting for user response to SELECT ITEM command.
---
 src/stk.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index bc46b2f..cda378f 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -115,6 +115,9 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
 	if (stk->driver->terminal_response == NULL)
 		return -ENOSYS;
 
+	if (stk->pending_cmd == NULL)
+		return -EINVAL;
+
 	rsp->src = STK_DEVICE_IDENTITY_TYPE_TERMINAL;
 	rsp->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
 	rsp->number = stk->pending_cmd->number;
-- 
1.7.0.4

Re: [PATCH] stk: Add null pointer check

[Andrzej Zaborowski]

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

Hi Lasse,

On 25 February 2011 11:39, Lasse Kunnasluoto
<lasse.kunnasluoto@tieto.com> wrote:
> Fixes a crash when SIM Session End is received while waiting for user response to SELECT ITEM command.

Can you post the ofono debug log for example?  It sounds like
something else is wrong because stk_respond should not be called when
the session is ended by the card.

Best regards

Re: [PATCH] stk: Add null pointer check

[Lasse Kunnasluoto]

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

Hi Andrzej,

On Fri, 2011-02-25 at 14:39 +0200, Andrzej Zaborowski wrote:
> Hi Lasse,
> 
> On 25 February 2011 11:39, Lasse Kunnasluoto
> <lasse.kunnasluoto@tieto.com> wrote:
> > Fixes a crash when SIM Session End is received while waiting for user response to SELECT ITEM command.
> 
> Can you post the ofono debug log for example?  It sounds like
> something else is wrong because stk_respond should not be called when
> the session is ended by the card.

Sure, log below.

For me it looks like modem times out after 1 min if no response given. I
don't think the session was terminated by SIM. Usually SIM cards wait
for TERMINAL RESPONSE basically forever. SIM has no mechanism to
terminate ongoing proactive command (well, except reset itself)

ofonod[3228]: src/stk.c:stk_select_item() 
ofonod[3228]: src/stk.c:stk_select_item() 
ofonod[3228]: src/stk.c:stk_send_envelope() 
ofonod[3228]: drivers/mbmmodem/stk.c:mbm_stk_envelope() 
ofonod[3228]: drivers/mbmmodem/stk.c:mbm_stk_envelope()
AT*STKE="D30782020181900101"
ofonod[3228]: SIM: > AT*STKE="D30782020181900101"\r
ofonod[3228]: SIM: < \r\nOK\r\n
ofonod[3228]: drivers/mbmmodem/stk.c:stke_cb() 
ofonod[3228]: src/stk.c:envelope_cb() length 0
ofonod[3228]: src/stk.c:menu_selection_envelope_cb() 
ofonod[3228]: src/stk.c:menu_selection_envelope_cb() Menu Selection
envelope submission gave no error
ofonod[3228]: SIM: < \r
\n*STKI:"D05681030124808202818205083E507265706169640F10224372656469742072656368617267650F07234372656469740F0B245361756E616C616874690F09254C616E67756167650F072650726963657318052313101324"\r\n
ofonod[3228]: drivers/mbmmodem/stk.c:stki_notify() 
ofonod[3228]: src/stk.c:stk_menu_create() 
ofonod[3228]: SIM: < \r\n*STKEND\r\n
ofonod[3228]: drivers/mbmmodem/stk.c:stkend_notify() 
ofonod[3228]: src/stk.c:session_agent_notify() Session Agent removed
ofonod[3228]: src/stk.c:session_agent_notify() Sending Terminate
response for session agent
ofonod[3228]: src/stk.c:send_simple_response() result 16
ofonod[3228]: src/stk.c:stk_respond() 
ofonod[3228]: Aborting (signal 11)
ofonod[3228]: ++++++++ backtrace ++++++++
ofonod[3228]: [0]: [0x4fe400]
ofonod[3228]: [1]: src/ofonod() [0x80dcfb8]
ofonod[3228]: [2]: src/ofonod() [0x80dd495]
ofonod[3228]: [3]: src/ofonod() [0x80f6e10]
ofonod[3228]: [4]: /lib/libglib-2.0.so.0(g_slist_foreach+0x27)
[0xe6b3d7]
ofonod[3228]: [5]: src/ofonod() [0x806fbe6]
ofonod[3228]: [6]: src/ofonod() [0x8071c17]
ofonod[3228]: [7]: /lib/libglib-2.0.so.0(+0x7fefb) [0xe8fefb]
ofonod[3228]: [8]: /lib/libglib-2.0.so.0(g_main_context_dispatch+0x1d5)
[0xe4b5e5]
ofonod[3228]: [9]: /lib/libglib-2.0.so.0(+0x3f2d8) [0xe4f2d8]
ofonod[3228]: [10]: /lib/libglib-2.0.so.0(g_main_loop_run+0x187)
[0xe4f817]
ofonod[3228]: [11]: src/ofonod() [0x80b5791]
ofonod[3228]: [12]: /lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe6)
[0x2d1bd6]
ofonod[3228]: [13]: src/ofonod() [0x8052f91]
ofonod[3228]: +++++++++++++++++++++++++++

BR,
-Lasse


> 
> Best regards

Re: [PATCH] stk: Add null pointer check

[Andrzej Zaborowski]

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

On 25 February 2011 14:04, Lasse Kunnasluoto
<lasse.kunnasluoto@tieto.com> wrote:
> On Fri, 2011-02-25 at 14:39 +0200, Andrzej Zaborowski wrote:
>> On 25 February 2011 11:39, Lasse Kunnasluoto <lasse.kunnasluoto@tieto.com> wrote:
>> > Fixes a crash when SIM Session End is received while waiting for user response to SELECT ITEM command.
>>
>> Can you post the ofono debug log for example?  It sounds like
>> something else is wrong because stk_respond should not be called when
>> the session is ended by the card.
>
> Sure, log below.
>
> For me it looks like modem times out after 1 min if no response given. I
> don't think the session was terminated by SIM. Usually SIM cards wait
> for TERMINAL RESPONSE basically forever. SIM has no mechanism to
> terminate ongoing proactive command (well, except reset itself)

Thanks for the log.

Even if it is the modem timing out, our assumption was that no
TERMINAL RESPONSE is wanted after the timeout (*STKEND).  So I think
the following would be a better fix:

diff --git a/src/stk.c b/src/stk.c
index bc46b2f..c4e988b 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -2514,6 +2514,7 @@ static void stk_proactive_command_cancel(struct
ofono_stk *stk)
                stk_command_free(stk->pending_cmd);
                stk->pending_cmd = NULL;
                stk->cancel_cmd = NULL;
+               stk->respond_on_exit = FALSE;
        }
 }

Alternatively this line can be added in stk_request_cancel

Best regards

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: reset-respond_on_exit.patch --]
[-- Type: text/x-patch, Size: 319 bytes --]

diff --git a/src/stk.c b/src/stk.c
index bc46b2f..c4e988b 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -2514,6 +2514,7 @@ static void stk_proactive_command_cancel(struct ofono_stk *stk)
 		stk_command_free(stk->pending_cmd);
 		stk->pending_cmd = NULL;
 		stk->cancel_cmd = NULL;
+		stk->respond_on_exit = FALSE;
 	}
 }
 

Re: [PATCH] stk: Add null pointer check

[Denis Kenzior]

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

Hi Andrew / Lasse,

> diff --git a/src/stk.c b/src/stk.c
> index bc46b2f..c4e988b 100644
> --- a/src/stk.c
> +++ b/src/stk.c
> @@ -2514,6 +2514,7 @@ static void stk_proactive_command_cancel(struct
> ofono_stk *stk)
>                 stk_command_free(stk->pending_cmd);
>                 stk->pending_cmd = NULL;
>                 stk->cancel_cmd = NULL;
> +               stk->respond_on_exit = FALSE;
>         }
>  }
> 

Patch looks good to me, however I'd like it in something I can actually
apply.

Lasse, can you confirm this patch solves your issue?

Regards,
-Denis

Re: [PATCH] stk: Add null pointer check

[Lasse Kunnasluoto]

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

Hi,

On Fri, 2011-02-25 at 19:58 +0200, Denis Kenzior wrote:
> Hi Andrew / Lasse,
> 
> > diff --git a/src/stk.c b/src/stk.c
> > index bc46b2f..c4e988b 100644
> > --- a/src/stk.c
> > +++ b/src/stk.c
> > @@ -2514,6 +2514,7 @@ static void stk_proactive_command_cancel(struct
> > ofono_stk *stk)
> >                 stk_command_free(stk->pending_cmd);
> >                 stk->pending_cmd = NULL;
> >                 stk->cancel_cmd = NULL;
> > +               stk->respond_on_exit = FALSE;
> >         }
> >  }
> > 
> 
> Patch looks good to me, however I'd like it in something I can actually
> apply.
> 
Andrew can probably provide a patch for this?

> Lasse, can you confirm this patch solves your issue?
Yes, it solves the issue. The null pointer check would be good to have
as well, but not mandatory if this is taken in

BR,
-Lasse

> 
> Regards,
> -Denis