* [PATCH BlueZ 1/2] Fixing possible use_after_free. @ 2018-07-20 4:33 Gopal Tiwari 2018-07-20 4:33 ` [PATCH BlueZ 2/2] Possible bug fix nsk replaced with sk Gopal Tiwari 2018-07-20 8:24 ` [PATCH BlueZ 1/2] Fixing possible use_after_free Luiz Augusto von Dentz 0 siblings, 2 replies; 7+ messages in thread From: Gopal Tiwari @ 2018-07-20 4:33 UTC (permalink / raw) To: linux-bluetooth; +Cc: dzickus, gtiwari 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH BlueZ 2/2] Possible bug fix nsk replaced with sk 2018-07-20 4:33 [PATCH BlueZ 1/2] Fixing possible use_after_free Gopal Tiwari @ 2018-07-20 4:33 ` Gopal Tiwari 2018-07-20 8:24 ` [PATCH BlueZ 1/2] Fixing possible use_after_free Luiz Augusto von Dentz 1 sibling, 0 replies; 7+ messages in thread From: Gopal Tiwari @ 2018-07-20 4:33 UTC (permalink / raw) To: linux-bluetooth; +Cc: dzickus, gtiwari 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ 1/2] Fixing possible use_after_free. 2018-07-20 4:33 [PATCH BlueZ 1/2] Fixing possible use_after_free Gopal Tiwari 2018-07-20 4:33 ` [PATCH BlueZ 2/2] Possible bug fix nsk replaced with sk Gopal Tiwari @ 2018-07-20 8:24 ` Luiz Augusto von Dentz 2018-07-23 6:28 ` Gopal Tiwari 1 sibling, 1 reply; 7+ messages in thread From: Luiz Augusto von Dentz @ 2018-07-20 8:24 UTC (permalink / raw) To: Gopal Tiwari; +Cc: linux-bluetooth, Don Zickus 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ 1/2] Fixing possible use_after_free. 2018-07-20 8:24 ` [PATCH BlueZ 1/2] Fixing possible use_after_free Luiz Augusto von Dentz @ 2018-07-23 6:28 ` Gopal Tiwari 2018-07-23 8:49 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 7+ messages in thread From: Gopal Tiwari @ 2018-07-23 6:28 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Don Zickus 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ 1/2] Fixing possible use_after_free. 2018-07-23 6:28 ` Gopal Tiwari @ 2018-07-23 8:49 ` Luiz Augusto von Dentz 2018-07-23 9:06 ` Gopal Tiwari 2018-07-23 14:25 ` Don Zickus 0 siblings, 2 replies; 7+ messages in thread From: Luiz Augusto von Dentz @ 2018-07-23 8:49 UTC (permalink / raw) To: Gopal Tiwari; +Cc: linux-bluetooth, Don Zickus 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ 1/2] Fixing possible use_after_free. 2018-07-23 8:49 ` Luiz Augusto von Dentz @ 2018-07-23 9:06 ` Gopal Tiwari 2018-07-23 14:25 ` Don Zickus 1 sibling, 0 replies; 7+ messages in thread From: Gopal Tiwari @ 2018-07-23 9:06 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Don Zickus >----- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ 1/2] Fixing possible use_after_free. 2018-07-23 8:49 ` Luiz Augusto von Dentz 2018-07-23 9:06 ` Gopal Tiwari @ 2018-07-23 14:25 ` Don Zickus 1 sibling, 0 replies; 7+ messages in thread From: Don Zickus @ 2018-07-23 14:25 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Gopal Tiwari, linux-bluetooth 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-23 14:25 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-20 4:33 [PATCH BlueZ 1/2] Fixing possible use_after_free Gopal Tiwari 2018-07-20 4:33 ` [PATCH BlueZ 2/2] Possible bug fix nsk replaced with sk Gopal Tiwari 2018-07-20 8:24 ` [PATCH BlueZ 1/2] Fixing possible use_after_free Luiz Augusto von Dentz 2018-07-23 6:28 ` Gopal Tiwari 2018-07-23 8:49 ` Luiz Augusto von Dentz 2018-07-23 9:06 ` Gopal Tiwari 2018-07-23 14:25 ` Don Zickus
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.