Re: [PATCH 2/3] message-waiting: add allocation checks

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Pekka,

On 01/21/2011 06:44 AM, Pekka.Pessi(a)nokia.com wrote:
> From: Pekka Pessi <Pekka.Pessi@nokia.com>
> 
> ---
>  src/message-waiting.c |   45 ++++++++++++++++++++++++++++++---------------
>  1 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/src/message-waiting.c b/src/message-waiting.c
> index 0e376b6..5d1126b 100644
> --- a/src/message-waiting.c
> +++ b/src/message-waiting.c
> @@ -179,20 +179,22 @@ static DBusMessage *set_cphs_mbdn(struct ofono_message_waiting *mw,
>  
>  	if ((mw->ef_cphs_mbdn_length && !mw_mailbox_to_cphs_record[mailbox]) ||
>  			mw->cphs_mbdn_not_provided == TRUE) {
> -		if (msg)
> +		if (msg != NULL)

Please send this as a separate style patch

>  			return __ofono_error_not_supported(msg);
>  
>  		return NULL;
>  	}
>  
>  	if (mw->ef_cphs_mbdn_length == 0) {
> -		if (msg)
> +		if (msg != NULL)

ditto

>  			return __ofono_error_sim_not_ready(msg);
>  
>  		return NULL;
>  	}
>  
>  	req = g_new0(struct mbdn_set_request, 1);
> +	if (req == NULL)
> +		goto error;

You don't actually need to perform a NULL check here.  g_new0 will never
return a NULL.  Only g_try_new0 will.

>  
>  	req->mw = mw;
>  	req->mailbox = mailbox;
> @@ -206,13 +208,17 @@ static DBusMessage *set_cphs_mbdn(struct ofono_message_waiting *mw,
>  			sync ? cphs_mbdn_sync_cb : mbdn_set_cb,
>  			OFONO_SIM_FILE_STRUCTURE_FIXED,
>  			mw_mailbox_to_cphs_record[mailbox],
> -			efmbdn, mw->ef_cphs_mbdn_length, req) == -1) {
> -		g_free(req);
> +			efmbdn, mw->ef_cphs_mbdn_length, req) == -1)
> +		goto error;
>  
> -		if (msg)
> -			return __ofono_error_failed(msg);
> -	} else
> -		req->msg = msg ? dbus_message_ref(msg) : NULL;
> +	req->msg = msg ? dbus_message_ref(msg) : NULL;
> +	return NULL;
> +
> +error:
> +	g_free(req);
> +
> +	if (msg != NULL)
> +		return __ofono_error_failed(msg);
>  
>  	return NULL;
>  }
> @@ -292,6 +298,8 @@ static DBusMessage *set_mbdn(struct ofono_message_waiting *mw, int mailbox,
>  	}
>  
>  	req = g_new0(struct mbdn_set_request, 1);
> +	if (req == NULL)
> +		goto error;

Same comment as above

>  
>  	req->mw = mw;
>  	req->mailbox = mailbox;
> @@ -303,13 +311,17 @@ static DBusMessage *set_mbdn(struct ofono_message_waiting *mw, int mailbox,
>  	if (ofono_sim_write(req->mw->sim, SIM_EFMBDN_FILEID, mbdn_set_cb,
>  			OFONO_SIM_FILE_STRUCTURE_FIXED,
>  			req->mw->efmbdn_record_id[mailbox],
> -			efmbdn, req->mw->efmbdn_length, req) == -1) {
> -		g_free(req);
> +			efmbdn, req->mw->efmbdn_length, req) == -1)
> +		goto error;
>  
> -		if (msg)
> -			return __ofono_error_failed(msg);
> -	} else
> -		req->msg = msg ? dbus_message_ref(msg) : NULL;
> +	req->msg = msg ? dbus_message_ref(msg) : NULL;
> +	return NULL;
> +
> +error:
> +	g_free(req);
> +
> +	if (msg)
> +		return __ofono_error_failed(msg);
>  
>  	return NULL;
>  }
> @@ -990,13 +1002,16 @@ struct ofono_message_waiting *ofono_message_waiting_create(struct ofono_modem *m
>  	struct ofono_message_waiting *mw;
>  
>  	mw = g_try_new0(struct ofono_message_waiting, 1);
> -

Please send this along with the style patch...

>  	if (mw == NULL)
>  		return NULL;
>  
>  	mw->atom = __ofono_modem_add_atom(modem,
>  					OFONO_ATOM_TYPE_MESSAGE_WAITING,
>  					mw_remove, mw);
> +	if (mw->atom == NULL) {
> +		g_free(mw);
> +		return NULL;
> +	}

This chunk is not necessary since add_atom does not use g_try_new...

>  
>  	return mw;
>  }

Regards,
-Denis