[PATCH] fix ofonod crash when pin is blocked

[PATCH] fix ofonod crash when pin is blocked

[caiwen.zhang]

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

From: Caiwen Zhang <caiwen.zhang@intel.com>

When is blocked, gprs/network atom is removed, it will
remove the SIM SPN watch which is added when it is created.
If at that time, SIM atom has been removed ofonod will crash
due to "sim->spn_watches" is NULL.

There is the same issue about network registration status watch.
---
 src/watch.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/watch.c b/src/watch.c
index dfb01fb..906c559 100644
--- a/src/watch.c
+++ b/src/watch.c
@@ -53,6 +53,9 @@ gboolean __ofono_watchlist_remove_item(struct ofono_watchlist *watchlist,
 	GSList *p;
 	GSList *c;
 
+	if (watchlist == NULL)
+		return;
+
 	p = NULL;
 	c = watchlist->items;
 
-- 
1.7.9.5

Re: [PATCH] fix ofonod crash when pin is blocked

[Denis Kenzior]

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

Hi Caiwen,

On 09/11/2013 12:13 PM, caiwen.zhang(a)intel.com wrote:
> From: Caiwen Zhang <caiwen.zhang@intel.com>
>
> When is blocked, gprs/network atom is removed, it will
> remove the SIM SPN watch which is added when it is created.
> If at that time, SIM atom has been removed ofonod will crash
> due to "sim->spn_watches" is NULL.
>

This description makes no sense.  The SIM atom is never removed due to a 
blocked PIN, so can you please provide a more clear description of the 
issue?

> There is the same issue about network registration status watch.
> ---
>   src/watch.c |    3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/src/watch.c b/src/watch.c
> index dfb01fb..906c559 100644
> --- a/src/watch.c
> +++ b/src/watch.c
> @@ -53,6 +53,9 @@ gboolean __ofono_watchlist_remove_item(struct ofono_watchlist *watchlist,
>   	GSList *p;
>   	GSList *c;
>
> +	if (watchlist == NULL)
> +		return;
> +

This fix is wrong, you're likely just covering up the symptom, not the 
cause.  oFono operates on the principle of crash-early, so that bugs are 
easier to detect and find.

What is the real reason why the watchlist is NULL at the point in time 
this function is being called?

>   	p = NULL;
>   	c = watchlist->items;
>
>

Regards,
-Denis

RE: [PATCH] fix ofonod crash when pin is blocked

[Zhang, Caiwen]

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

Hi Denis,

> > When is blocked, gprs/network atom is removed, it will remove the SIM
> > SPN watch which is added when it is created.
> > If at that time, SIM atom has been removed ofonod will crash due to
> > "sim->spn_watches" is NULL.
> >
> 
> This description makes no sense.  The SIM atom is never removed due to a
> blocked PIN, so can you please provide a more clear description of the issue?
> 
> > There is the same issue about network registration status watch.
> > ---
> >   src/watch.c |    3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/src/watch.c b/src/watch.c index dfb01fb..906c559 100644
> > --- a/src/watch.c
> > +++ b/src/watch.c
> > @@ -53,6 +53,9 @@ gboolean __ofono_watchlist_remove_item(struct
> ofono_watchlist *watchlist,
> >   	GSList *p;
> >   	GSList *c;
> >
> > +	if (watchlist == NULL)
> > +		return;
> > +
> 
> This fix is wrong, you're likely just covering up the symptom, not the cause.
> oFono operates on the principle of crash-early, so that bugs are easier to detect
> and find.
> 
> What is the real reason why the watchlist is NULL at the point in time this
> function is being called?

Yes, you are right. The correct comment should be "If at that time, "spn_watch" has been removed ofonod will crash due to
"sim->spn_watches" is NULL."

"spn_watch" is removed at "sim_free_main_state" function when SIM PIN is blocked. This is done before gprs/network atom is removed.

About network registration status watch, the original comment is true.

Regards,
Caiwen

Re: [PATCH] fix ofonod crash when pin is blocked

[Denis Kenzior]

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

Hi Caiwen,

 >>
>> What is the real reason why the watchlist is NULL at the point in time this
>> function is being called?
>
> Yes, you are right. The correct comment should be "If at that time, "spn_watch" has been removed ofonod will crash due to
> "sim->spn_watches" is NULL."
>

Then please fix the patch description

> "spn_watch" is removed at "sim_free_main_state" function when SIM PIN is blocked. This is done before gprs/network atom is removed.
>

Then please fix the underlying issue itself.  Would signaling LOCKED_OUT 
state first before freeing the main state do the trick?


> About network registration status watch, the original comment is true.

Please elaborate.

Regards,
-Denis