diff mbox

[07/18] P2P: Fix possible memory leak in p2p_group_delete

Message ID 1473085991-5073-7-git-send-email-andrei.otcheretianski@intel.com
State Changes Requested
Headers show

Commit Message

Andrei Otcheretianski Sept. 5, 2016, 2:33 p.m. UTC 
From: Ilan Peer <ilan.peer@intel.com>

In wpas_p2p_group_delete() free the pointer earlier, as the function
might return early.

Signed-off-by: Ilan Peer <ilan.peer@intel.com>
---
 wpa_supplicant/p2p_supplicant.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jouni Malinen Sept. 10, 2016, 6:44 p.m. UTC | #1 
On Mon, Sep 05, 2016 at 05:33:00PM +0300, andrei.otcheretianski@intel.com wrote:
> In wpas_p2p_group_delete() free the pointer earlier, as the function
> might return early.

> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> @@ -929,6 +929,10 @@ static int wpas_p2p_group_delete(struct wpa_supplicant *wpa_s,
> +	os_free(wpa_s->p2p_group_common_freqs);
> +	wpa_s->p2p_group_common_freqs = NULL;
> +	wpa_s->p2p_group_common_freqs_num = 0;

Hmm.. How exactly would this be fixing a memory leak when
wpa_s->p2p_group_common_freqs is freed in wpas_p2p_deinit() which would
be called if the group interface got removed before reached the later
point down here:

> @@ -968,10 +972,6 @@ static int wpas_p2p_group_delete(struct wpa_supplicant *wpa_s,
>  	os_free(wpa_s->go_params);
>  	wpa_s->go_params = NULL;
>  
> -	os_free(wpa_s->p2p_group_common_freqs);
> -	wpa_s->p2p_group_common_freqs = NULL;
> -	wpa_s->p2p_group_common_freqs_num = 0;

And if wpa_s->p2p_group_common_freqs would have a memory leak, wouldn't
wpa_s->go_params be in the same category? (And that is also freed in
wpas_p2p_deinit() to make it not leak if wpas_p2p_group_delete() returns
earlier..)
Peer, Ilan Sept. 11, 2016, 12:50 p.m. UTC | #2 
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Saturday, September 10, 2016 21:45
> To: Otcheretianski, Andrei <andrei.otcheretianski@intel.com>
> Cc: hostap@lists.infradead.org; Peer, Ilan <ilan.peer@intel.com>
> Subject: Re: [PATCH 07/18] P2P: Fix possible memory leak in
> p2p_group_delete
> 
> On Mon, Sep 05, 2016 at 05:33:00PM +0300, andrei.otcheretianski@intel.com
> wrote:
> > In wpas_p2p_group_delete() free the pointer earlier, as the function
> > might return early.
> 
> > diff --git a/wpa_supplicant/p2p_supplicant.c
> > b/wpa_supplicant/p2p_supplicant.c @@ -929,6 +929,10 @@ static int
> > wpas_p2p_group_delete(struct wpa_supplicant *wpa_s,
> > +	os_free(wpa_s->p2p_group_common_freqs);
> > +	wpa_s->p2p_group_common_freqs = NULL;
> > +	wpa_s->p2p_group_common_freqs_num = 0;
> 
> Hmm.. How exactly would this be fixing a memory leak when wpa_s-
> >p2p_group_common_freqs is freed in wpas_p2p_deinit() which would be
> called if the group interface got removed before reached the later point
> down here:
> 

Agree. Please drop this patch. 

Thanks,

Ilan.
diff mbox

Patch

diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index c138413..21b6213 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -929,6 +929,10 @@  static int wpas_p2p_group_delete(struct wpa_supplicant *wpa_s,
 	eloop_cancel_timeout(wpas_p2p_move_go, wpa_s, NULL);
 	eloop_cancel_timeout(wpas_p2p_reconsider_moving_go, wpa_s, NULL);
 
+	os_free(wpa_s->p2p_group_common_freqs);
+	wpa_s->p2p_group_common_freqs = NULL;
+	wpa_s->p2p_group_common_freqs_num = 0;
+
 	/*
 	 * Make sure wait for the first client does not remain active after the
 	 * group has been removed.
@@ -968,10 +972,6 @@  static int wpas_p2p_group_delete(struct wpa_supplicant *wpa_s,
 	os_free(wpa_s->go_params);
 	wpa_s->go_params = NULL;
 
-	os_free(wpa_s->p2p_group_common_freqs);
-	wpa_s->p2p_group_common_freqs = NULL;
-	wpa_s->p2p_group_common_freqs_num = 0;
-
 	wpa_s->waiting_presence_resp = 0;
 
 	wpa_printf(MSG_DEBUG, "P2P: Remove temporary group network");