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@xxxxxxxxxx> 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@xxxxxxxxx> > > To: Gopal Tiwari <gtiwari@xxxxxxxxxx> > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx, Don Zickus <dzickus@xxxxxxxxxx> > > 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx > >> 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
