[PATCH BlueZ 1/2] Fixing possible use_after_free.

[PATCH BlueZ 1/2] Fixing possible use_after_free.

[Gopal Tiwari]

During the code review of code. Found some of the possible
double free scenario's. So fixing them.
---
 gobex/gobex-transfer.c | 6 ++++++
 obexd/client/session.c | 6 +++++-
 tools/mesh/prov-db.c   | 4 +++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
index bc99306..11980d0 100644
--- a/gobex/gobex-transfer.c
+++ b/gobex/gobex-transfer.c
@@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
 	va_start(args, first_hdr_id);
 	transfer_put_req_first(transfer, req, first_hdr_id, args);
 	va_end(args);
+
+	/* Checking transfer in case transfer_put_req_first has
+	 * freed it to avoid double free */
+	if(!transfer)
+		return 0;
+
 	if (!g_slist_find(transfers, transfer))
 		return 0;
 
diff --git a/obexd/client/session.c b/obexd/client/session.c
index 5bd2d26..077224b 100644
--- a/obexd/client/session.c
+++ b/obexd/client/session.c
@@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
 	if (session->p == NULL)
 		session_process_queue(session);
 
-	obc_session_unref(session);
+	/* Verifying the session variable for double free
+	 */
+
+	if (session)
+		obc_session_unref(session);
 }
 
 static void session_notify_complete(struct obc_session *session,
diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
index 05b2547..74915d2 100644
--- a/tools/mesh/prov-db.c
+++ b/tools/mesh/prov-db.c
@@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
 
 	res = true;
 done:
+	/* Varify in_str for double free */
 
-	g_free(in_str);
+	if(in_str)
+		g_free(in_str);
 
 	if(jmain)
 		json_object_put(jmain);
-- 
2.7.5

[PATCH BlueZ 2/2] Possible bug fix nsk replaced with sk

[Gopal Tiwari]

During the review of the code path found this scenario where
seems like nsk is replaced with sk. Where as sk is already free.
Fixing it.
---
 tools/rctest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/rctest.c b/tools/rctest.c
index 6d84e07..909bc68 100644
--- a/tools/rctest.c
+++ b/tools/rctest.c
@@ -376,7 +376,7 @@ static void do_listen(void (*handler)(int sk))
 			//goto error;
 		}
 
-		if (priority > 0 && setsockopt(sk, SOL_SOCKET, SO_PRIORITY,
+		if (priority > 0 && setsockopt(nsk, SOL_SOCKET, SO_PRIORITY,
 					&priority, sizeof(priority)) < 0) {
 			syslog(LOG_ERR, "Can't set socket priority: %s (%d)",
 						strerror(errno), errno);
-- 
2.7.5

Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

[Luiz Augusto von Dentz]

Hi Gopal,

On Fri, Jul 20, 2018 at 7:33 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> During the code review of code. Found some of the possible
> double free scenario's. So fixing them.

Id like that you split the changes with possible traces when that
happen, besides some changes seems questionable (see the comments
bellow).

> ---
>  gobex/gobex-transfer.c | 6 ++++++
>  obexd/client/session.c | 6 +++++-
>  tools/mesh/prov-db.c   | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
> index bc99306..11980d0 100644
> --- a/gobex/gobex-transfer.c
> +++ b/gobex/gobex-transfer.c
> @@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
>         va_start(args, first_hdr_id);
>         transfer_put_req_first(transfer, req, first_hdr_id, args);
>         va_end(args);
> +
> +       /* Checking transfer in case transfer_put_req_first has
> +        * freed it to avoid double free */
> +       if(!transfer)
> +               return 0;

Check the code style, the should be a space after like in: if (

>         if (!g_slist_find(transfers, transfer))
>                 return 0;
>
> diff --git a/obexd/client/session.c b/obexd/client/session.c
> index 5bd2d26..077224b 100644
> --- a/obexd/client/session.c
> +++ b/obexd/client/session.c
> @@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
>         if (session->p == NULL)
>                 session_process_queue(session);
>
> -       obc_session_unref(session);
> +       /* Verifying the session variable for double free
> +        */
> +
> +       if (session)
> +               obc_session_unref(session);

I don't think you are fixing anything here with checking the session
pointer since any function altering it won't be able to reset the
pointer to NULL. If is a chance of double free here then what should
be done is have a extra reference to prevent it to be free before but
given the function name that should never happen so I wonder where you
got this from?

>  }
>
>  static void session_notify_complete(struct obc_session *session,
> diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
> index 05b2547..74915d2 100644
> --- a/tools/mesh/prov-db.c
> +++ b/tools/mesh/prov-db.c
> @@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
>
>         res = true;
>  done:
> +       /* Varify in_str for double free */
>
> -       g_free(in_str);
> +       if(in_str)
> +               g_free(in_str);

g_free is already NULL pointer safe.

>
>         if(jmain)
>                 json_object_put(jmain);
> --
> 2.7.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

[Gopal Tiwari]

Hi Luiz, 

We are using a tool to review code and generate warning as an when it sees any of the problem scenarios like use_after_free etc.. 
 
----- Original Message -----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Gopal Tiwari <gtiwari@redhat.com>
Cc: linux-bluetooth@vger.kernel.org, Don Zickus <dzickus@redhat.com>
Sent: Fri, 20 Jul 2018 04:24:34 -0400 (EDT)
Subject: Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

>Hi Gopal,

>On Fri, Jul 20, 2018 at 7:33 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> During the code review of code. Found some of the possible
> double free scenario's. So fixing them.

>Id like that you split the changes with possible traces when that
>happen, besides some changes seems questionable (see the comments
>bellow).

> ---
>  gobex/gobex-transfer.c | 6 ++++++
>  obexd/client/session.c | 6 +++++-
>  tools/mesh/prov-db.c   | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
> index bc99306..11980d0 100644
> --- a/gobex/gobex-transfer.c
> +++ b/gobex/gobex-transfer.c
> @@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
>         va_start(args, first_hdr_id);
>         transfer_put_req_first(transfer, req, first_hdr_id, args);
>         va_end(args);
> +
> +       /* Checking transfer in case transfer_put_req_first has
> +        * freed it to avoid double free */
> +       if(!transfer)
> +               return 0;

>Check the code style, the should be a space after like in: if (

My mistake :(. I'll re-post it. Thanks for pointing out.   

>         if (!g_slist_find(transfers, transfer))
>                 return 0;
>
> diff --git a/obexd/client/session.c b/obexd/client/session.c
> index 5bd2d26..077224b 100644
> --- a/obexd/client/session.c
> +++ b/obexd/client/session.c
> @@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
>         if (session->p == NULL)
>                 session_process_queue(session);
>
> -       obc_session_unref(session);
> +       /* Verifying the session variable for double free
> +        */
> +
> +       if (session)
> +               obc_session_unref(session);

This is how we have got this .. 

bluez-5.49/obexd/client/session.c:935: freed_arg: "session_process_queue" frees "session".
bluez-5.49/obexd/client/session.c:894:2: freed_arg: "obc_session_unref" frees parameter "session".
bluez-5.49/obexd/client/session.c:304:2: freed_arg: "session_free" frees parameter "session".
bluez-5.49/obexd/client/session.c:252:2: freed_arg: "g_free" frees parameter "session".
bluez-5.49/obexd/client/session.c:937: deref_arg: Calling "obc_session_unref" dereferences freed pointer "session".
bluez-5.49/obexd/client/session.c:287:2: deref_parm: Directly dereferencing parameter "session".
#  935|   		session_process_queue(session);
#  936|   
#  937|-> 	obc_session_unref(session);


>I don't think you are fixing anything here with checking the session
>pointer since any function altering it won't be able to reset the
>pointer to NULL. If is a chance of double free here then what should
>be done is have a extra reference to prevent it to be free before but
>given the function name that should never happen so I wonder where you
>got this from?

>  }
>
>  static void session_notify_complete(struct obc_session *session,
> diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
> index 05b2547..74915d2 100644
> --- a/tools/mesh/prov-db.c
> +++ b/tools/mesh/prov-db.c
> @@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
>
>         res = true;
>  done:
> +       /* Varify in_str for double free */
>
> -       g_free(in_str);
> +       if(in_str)
> +               g_free(in_str);

bluez-5.49/mesh/prov-db.c:860: freed_arg: "g_free" frees "in_str".
bluez-5.49/mesh/prov-db.c:880: double_free: Calling "g_free" frees pointer "in_str" which has already been freed.
#  878|   done:
#  879|   
#  880|-> 	g_free(in_str);

>g_free is already NULL pointer safe.

Ok, I forgot to check that :( my fault. 

>
>         if(jmain)
>                 json_object_put(jmain);
> --
> 2.7.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gopal..

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

[Luiz Augusto von Dentz]

Hi Gopal,

On Mon, Jul 23, 2018 at 9:28 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> Hi Luiz,
>
> We are using a tool to review code and generate warning as an when it sees any of the problem scenarios like use_after_free etc..
>
> ----- Original Message -----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> To: Gopal Tiwari <gtiwari@redhat.com>
> Cc: linux-bluetooth@vger.kernel.org, Don Zickus <dzickus@redhat.com>
> Sent: Fri, 20 Jul 2018 04:24:34 -0400 (EDT)
> Subject: Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.
>
>>Hi Gopal,
>
>>On Fri, Jul 20, 2018 at 7:33 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
>> During the code review of code. Found some of the possible
>> double free scenario's. So fixing them.
>
>>Id like that you split the changes with possible traces when that
>>happen, besides some changes seems questionable (see the comments
>>bellow).
>
>> ---
>>  gobex/gobex-transfer.c | 6 ++++++
>>  obexd/client/session.c | 6 +++++-
>>  tools/mesh/prov-db.c   | 4 +++-
>>  3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
>> index bc99306..11980d0 100644
>> --- a/gobex/gobex-transfer.c
>> +++ b/gobex/gobex-transfer.c
>> @@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
>>         va_start(args, first_hdr_id);
>>         transfer_put_req_first(transfer, req, first_hdr_id, args);
>>         va_end(args);
>> +
>> +       /* Checking transfer in case transfer_put_req_first has
>> +        * freed it to avoid double free */
>> +       if(!transfer)
>> +               return 0;
>
>>Check the code style, the should be a space after like in: if (
>
> My mistake :(. I'll re-post it. Thanks for pointing out.
>
>>         if (!g_slist_find(transfers, transfer))
>>                 return 0;
>>
>> diff --git a/obexd/client/session.c b/obexd/client/session.c
>> index 5bd2d26..077224b 100644
>> --- a/obexd/client/session.c
>> +++ b/obexd/client/session.c
>> @@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
>>         if (session->p == NULL)
>>                 session_process_queue(session);
>>
>> -       obc_session_unref(session);
>> +       /* Verifying the session variable for double free
>> +        */
>> +
>> +       if (session)
>> +               obc_session_unref(session);
>
> This is how we have got this ..
>
> bluez-5.49/obexd/client/session.c:935: freed_arg: "session_process_queue" frees "session".
> bluez-5.49/obexd/client/session.c:894:2: freed_arg: "obc_session_unref" frees parameter "session".
> bluez-5.49/obexd/client/session.c:304:2: freed_arg: "session_free" frees parameter "session".
> bluez-5.49/obexd/client/session.c:252:2: freed_arg: "g_free" frees parameter "session".
> bluez-5.49/obexd/client/session.c:937: deref_arg: Calling "obc_session_unref" dereferences freed pointer "session".
> bluez-5.49/obexd/client/session.c:287:2: deref_parm: Directly dereferencing parameter "session".
> #  935|                 session_process_queue(session);
> #  936|
> #  937|->       obc_session_unref(session);

If that is happening we have a reference problem in
session_process_queue, that should never cause a session_free, and
obviously checking for the pointer on the caller function doesn't work
since that is never changed. Both function do actually unref their own
reference so either the session is already invalid to begin with or
perhaps some callback is unrefing when it shouldn't.

>
>>I don't think you are fixing anything here with checking the session
>>pointer since any function altering it won't be able to reset the
>>pointer to NULL. If is a chance of double free here then what should
>>be done is have a extra reference to prevent it to be free before but
>>given the function name that should never happen so I wonder where you
>>got this from?
>
>>  }
>>
>>  static void session_notify_complete(struct obc_session *session,
>> diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
>> index 05b2547..74915d2 100644
>> --- a/tools/mesh/prov-db.c
>> +++ b/tools/mesh/prov-db.c
>> @@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
>>
>>         res = true;
>>  done:
>> +       /* Varify in_str for double free */
>>
>> -       g_free(in_str);
>> +       if(in_str)
>> +               g_free(in_str);
>
> bluez-5.49/mesh/prov-db.c:860: freed_arg: "g_free" frees "in_str".
> bluez-5.49/mesh/prov-db.c:880: double_free: Calling "g_free" frees pointer "in_str" which has already been freed.
> #  878|   done:
> #  879|
> #  880|->       g_free(in_str);
>
>>g_free is already NULL pointer safe.
>
> Ok, I forgot to check that :( my fault.
>
>>
>>         if(jmain)
>>                 json_object_put(jmain);
>> --
>> 2.7.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Gopal..
>
> --
> Luiz Augusto von Dentz
>



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

[Gopal Tiwari]

>----- Original Message -----
>From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
>To: Gopal Tiwari <gtiwari@redhat.com>
>Cc: linux-bluetooth@vger.kernel.org, Don Zickus <dzickus@redhat.com>
>Sent: Mon, 23 Jul 2018 04:49:45 -0400 (EDT)
>Subject: Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

>Hi Gopal,

On Mon, Jul 23, 2018 at 9:28 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> Hi Luiz,
>
> We are using a tool to review code and generate warning as an when it sees any of the problem scenarios like use_after_free etc..
>
> ----- Original Message -----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> To: Gopal Tiwari <gtiwari@redhat.com>
> Cc: linux-bluetooth@vger.kernel.org, Don Zickus <dzickus@redhat.com>
> Sent: Fri, 20 Jul 2018 04:24:34 -0400 (EDT)
> Subject: Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.
>
>>Hi Gopal,
>
>>On Fri, Jul 20, 2018 at 7:33 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
>> During the code review of code. Found some of the possible
>> double free scenario's. So fixing them.
>
>>Id like that you split the changes with possible traces when that
>>happen, besides some changes seems questionable (see the comments
>>bellow).
>
>> ---
>>  gobex/gobex-transfer.c | 6 ++++++
>>  obexd/client/session.c | 6 +++++-
>>  tools/mesh/prov-db.c   | 4 +++-
>>  3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
>> index bc99306..11980d0 100644
>> --- a/gobex/gobex-transfer.c
>> +++ b/gobex/gobex-transfer.c
>> @@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
>>         va_start(args, first_hdr_id);
>>         transfer_put_req_first(transfer, req, first_hdr_id, args);
>>         va_end(args);
>> +
>> +       /* Checking transfer in case transfer_put_req_first has
>> +        * freed it to avoid double free */
>> +       if(!transfer)
>> +               return 0;
>
>>Check the code style, the should be a space after like in: if (
>
> My mistake :(. I'll re-post it. Thanks for pointing out.
>
>>         if (!g_slist_find(transfers, transfer))
>>                 return 0;
>>
>> diff --git a/obexd/client/session.c b/obexd/client/session.c
>> index 5bd2d26..077224b 100644
>> --- a/obexd/client/session.c
>> +++ b/obexd/client/session.c
>> @@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
>>         if (session->p == NULL)
>>                 session_process_queue(session);
>>
>> -       obc_session_unref(session);
>> +       /* Verifying the session variable for double free
>> +        */
>> +
>> +       if (session)
>> +               obc_session_unref(session);
>
> This is how we have got this ..
>
> bluez-5.49/obexd/client/session.c:935: freed_arg: "session_process_queue" frees "session".
> bluez-5.49/obexd/client/session.c:894:2: freed_arg: "obc_session_unref" frees parameter "session".
> bluez-5.49/obexd/client/session.c:304:2: freed_arg: "session_free" frees parameter "session".
> bluez-5.49/obexd/client/session.c:252:2: freed_arg: "g_free" frees parameter "session".
> bluez-5.49/obexd/client/session.c:937: deref_arg: Calling "obc_session_unref" dereferences freed pointer "session".
> bluez-5.49/obexd/client/session.c:287:2: deref_parm: Directly dereferencing parameter "session".
> #  935|                 session_process_queue(session);
> #  936|
> #  937|->       obc_session_unref(session);

>If that is happening we have a reference problem in
>session_process_queue, that should never cause a session_free, and
>obviously checking for the pointer on the caller function doesn't work
>since that is never changed.

Sure will remove following from my patch 

+       /* Verifying the session variable for double free
+        */
+
+       if (session)
+               obc_session_unref(session);


>Both function do actually unref their own
>reference so either the session is already invalid to begin with or
>perhaps some callback is unrefing when it shouldn't.

Ok, sure will try to take a look at  the call path again.

Gopal..

>
>>I don't think you are fixing anything here with checking the session
>>pointer since any function altering it won't be able to reset the
>>pointer to NULL. If is a chance of double free here then what should
>>be done is have a extra reference to prevent it to be free before but
>>given the function name that should never happen so I wonder where you
>>got this from?
>
>>  }
>>
>>  static void session_notify_complete(struct obc_session *session,
>> diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
>> index 05b2547..74915d2 100644
>> --- a/tools/mesh/prov-db.c
>> +++ b/tools/mesh/prov-db.c
>> @@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
>>
>>         res = true;
>>  done:
>> +       /* Varify in_str for double free */
>>
>> -       g_free(in_str);
>> +       if(in_str)
>> +               g_free(in_str);
>
> bluez-5.49/mesh/prov-db.c:860: freed_arg: "g_free" frees "in_str".
> bluez-5.49/mesh/prov-db.c:880: double_free: Calling "g_free" frees pointer "in_str" which has already been freed.
> #  878|   done:
> #  879|
> #  880|->       g_free(in_str);
>
>>g_free is already NULL pointer safe.
>
> Ok, I forgot to check that :( my fault.
>
>>
>>         if(jmain)
>>                 json_object_put(jmain);
>> --
>> 2.7.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Gopal..
>
> --
> Luiz Augusto von Dentz
>



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

[Don Zickus]

On Mon, Jul 23, 2018 at 11:49:45AM +0300, Luiz Augusto von Dentz wrote:
> Hi Gopal,
> 
> On Mon, Jul 23, 2018 at 9:28 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> > Hi Luiz,
> >
> > We are using a tool to review code and generate warning as an when it sees any of the problem scenarios like use_after_free etc..
> >
> > ----- Original Message -----
> > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> > To: Gopal Tiwari <gtiwari@redhat.com>
> > Cc: linux-bluetooth@vger.kernel.org, Don Zickus <dzickus@redhat.com>
> > Sent: Fri, 20 Jul 2018 04:24:34 -0400 (EDT)
> > Subject: Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.
> >
> >>Hi Gopal,
> >
> >>On Fri, Jul 20, 2018 at 7:33 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> >> During the code review of code. Found some of the possible
> >> double free scenario's. So fixing them.
> >
> >>Id like that you split the changes with possible traces when that
> >>happen, besides some changes seems questionable (see the comments
> >>bellow).
> >
> >> ---
> >>  gobex/gobex-transfer.c | 6 ++++++
> >>  obexd/client/session.c | 6 +++++-
> >>  tools/mesh/prov-db.c   | 4 +++-
> >>  3 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
> >> index bc99306..11980d0 100644
> >> --- a/gobex/gobex-transfer.c
> >> +++ b/gobex/gobex-transfer.c
> >> @@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
> >>         va_start(args, first_hdr_id);
> >>         transfer_put_req_first(transfer, req, first_hdr_id, args);
> >>         va_end(args);
> >> +
> >> +       /* Checking transfer in case transfer_put_req_first has
> >> +        * freed it to avoid double free */
> >> +       if(!transfer)
> >> +               return 0;
> >
> >>Check the code style, the should be a space after like in: if (
> >
> > My mistake :(. I'll re-post it. Thanks for pointing out.
> >
> >>         if (!g_slist_find(transfers, transfer))
> >>                 return 0;
> >>
> >> diff --git a/obexd/client/session.c b/obexd/client/session.c
> >> index 5bd2d26..077224b 100644
> >> --- a/obexd/client/session.c
> >> +++ b/obexd/client/session.c
> >> @@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
> >>         if (session->p == NULL)
> >>                 session_process_queue(session);
> >>
> >> -       obc_session_unref(session);
> >> +       /* Verifying the session variable for double free
> >> +        */
> >> +
> >> +       if (session)
> >> +               obc_session_unref(session);
> >
> > This is how we have got this ..
> >
> > bluez-5.49/obexd/client/session.c:935: freed_arg: "session_process_queue" frees "session".
> > bluez-5.49/obexd/client/session.c:894:2: freed_arg: "obc_session_unref" frees parameter "session".
> > bluez-5.49/obexd/client/session.c:304:2: freed_arg: "session_free" frees parameter "session".
> > bluez-5.49/obexd/client/session.c:252:2: freed_arg: "g_free" frees parameter "session".
> > bluez-5.49/obexd/client/session.c:937: deref_arg: Calling "obc_session_unref" dereferences freed pointer "session".
> > bluez-5.49/obexd/client/session.c:287:2: deref_parm: Directly dereferencing parameter "session".
> > #  935|                 session_process_queue(session);
> > #  936|
> > #  937|->       obc_session_unref(session);
> 
> If that is happening we have a reference problem in
> session_process_queue, that should never cause a session_free, and
> obviously checking for the pointer on the caller function doesn't work
> since that is never changed. Both function do actually unref their own
> reference so either the session is already invalid to begin with or
> perhaps some callback is unrefing when it shouldn't.

BTW, the tool Gopal is referencing is the Coverity tool.  Though it has been
getting better over the years, it can still occasionally get confused on
complex code paths.

Cheers,
Don

> 
> >
> >>I don't think you are fixing anything here with checking the session
> >>pointer since any function altering it won't be able to reset the
> >>pointer to NULL. If is a chance of double free here then what should
> >>be done is have a extra reference to prevent it to be free before but
> >>given the function name that should never happen so I wonder where you
> >>got this from?
> >
> >>  }
> >>
> >>  static void session_notify_complete(struct obc_session *session,
> >> diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
> >> index 05b2547..74915d2 100644
> >> --- a/tools/mesh/prov-db.c
> >> +++ b/tools/mesh/prov-db.c
> >> @@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
> >>
> >>         res = true;
> >>  done:
> >> +       /* Varify in_str for double free */
> >>
> >> -       g_free(in_str);
> >> +       if(in_str)
> >> +               g_free(in_str);
> >
> > bluez-5.49/mesh/prov-db.c:860: freed_arg: "g_free" frees "in_str".
> > bluez-5.49/mesh/prov-db.c:880: double_free: Calling "g_free" frees pointer "in_str" which has already been freed.
> > #  878|   done:
> > #  879|
> > #  880|->       g_free(in_str);
> >
> >>g_free is already NULL pointer safe.
> >
> > Ok, I forgot to check that :( my fault.
> >
> >>
> >>         if(jmain)
> >>                 json_object_put(jmain);
> >> --
> >> 2.7.5
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> > Gopal..
> >
> > --
> > Luiz Augusto von Dentz
> >
> 
> 
> 
> -- 
> Luiz Augusto von Dentz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html