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); >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 -- 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
