Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Gal Ben Haim <gbenhaim@augury.com>
Cc: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting
Date: Tue, 1 Jan 2019 16:27:13 -0300
Message-ID: <CABBYNZ+sPBv8QdMNp7-fTD_d9g2atNWka1whhD1iUas4g1JWKA@mail.gmail.com> (raw)
In-Reply-To: <CAHotPr9tdmnFhZLX2utqFxeXUx_0G0AYsfwKrmktV4USbfBcyw@mail.gmail.com>

Hi Gal,

On Sat, Dec 22, 2018 at 3:51 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
>
> it definitely happens with this patch and not with mine

Can you try with valgrind so we got to see if the backtrace is really
the same, also is that using GATT caching or is that disabled?

> On Sat, Dec 15, 2018 at 8:45 AM Gal Ben Haim <gbenhaim@augury.com> wrote:
> >
> > I still get segmentation faults occasionally after applying this patch,
> > i'm not 100% sure that it's related but it happens in the same area...
> > its not something that I can reproduce immediately like with the
> > previous segfault. I think that it doesn't happen if I use my original
> > patch https://marc.info/?l=linux-bluetooth&m=154250398526173&w=2
> >
> > bluetoothd[2599]: src/device.c:att_disconnected_cb()
> > bluetoothd[2599]: src/device.c:att_disconnected_cb() Connection reset
> > by peer (104)
> > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> > DA:51:61:24:3C:36 profile gap-profile state changed: connected ->
> > disconnecting (0)
> > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> > DA:51:61:24:3C:36 profile gap-profile state changed: disconnecting ->
> > disconnected (0)
> > bluetoothd[2599]: src/gatt-client.c:btd_gatt_client_disconnected()
> > Device disconnected. Cleaning up.
> > bluetoothd[2599]: src/device.c:att_disconnected_cb() Automatic
> > connection disabled
> > bluetoothd[2599]: src/gatt-database.c:btd_gatt_database_att_disconnected()
> > bluetoothd[2599]: attrib/gattrib.c:g_attrib_unref() 0x4ee410: g_attrib_unref=0
> > bluetoothd[2599]: src/device.c:btd_device_set_temporary() temporary 1
> > bluetoothd[2599]: src/device.c:device_remove() Removing device
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36
> > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> > DA:51:61:24:3C:36 profile gap-profile state changed: disconnected ->
> > unavailable (0)
> > bluetoothd[2599]: profiles/gap/gas.c:gap_remove() GAP profile remove
> > (DA:51:61:24:3C:36)
> > bluetoothd[2599]: src/service.c:btd_service_unref() 0x4edae0: ref=0
> > bluetoothd[2599]: src/device.c:btd_device_unref() Freeing device
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36
> > bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
> > service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0008
> > bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
> > service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009
> > bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
> > Removing GATT characteristic:
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000a
> > bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
> > Removing GATT characteristic:
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c
> > bluetoothd[2599]: src/gatt-client.c:unregister_descriptor() Removing
> > GATT descriptor:
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c/desc000e
> > Segmentation fault
> >
> >
> > On Wed, Nov 21, 2018 at 11:15 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi,
> > > On Wed, Nov 21, 2018 at 9:15 AM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > > >
> > > > it fixes the same case that cause the version from master to crash.
> > > > i haven't tested for anything else..
> > > >
> > > > On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > > > >
> > > > > ok I patched it. i'll test it tomorrow in the office and let you know.
> > > > >
> > > > > On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
> > > > > <luiz.dentz@gmail.com> wrote:
> > > > > >
> > > > > > Hi Gal,
> > > > > >
> > > > > > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > > > > > >
> > > > > > > i tried with both git am and git apply
> > > > > >
> > > > > > Does that applies to v2 as well? I just rebased it on top master, are
> > > > > > you sure you don't have your own changes conflicting with it?
> > > > > >
> > > > > > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > > > > > > <luiz.dentz@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Gal,
> > > > > > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > > > > > > > >
> > > > > > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > > > > > > >
> > > > > > > > They are on top o master, how are you trying to apply them, with git am?
> > > > > > > >
> > > > > > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > > > > > > <luiz.dentz@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > > > > > > <luiz.dentz@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > > > > > > >
> > > > > > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > > > > > > code not only have to free the client object but also destroy the io
> > > > > > > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > > > > > > >
> > > > > > > > > > > Invalid read of size 4
> > > > > > > > > > >    at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > > > > > >    by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > > > > > >    by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > > > > > >    by 0x81F33: remove_interface (object.c:667)
> > > > > > > > > > >    by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > > > > > >    by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > >    by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > > >    by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > > >    by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > > >    by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > > >    by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > > >    by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > > >    by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > > >  Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > > > > > >    at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > > > > > >    by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > >    by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > > > > > >    by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > >    by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > > >    by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > > >    by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > > >    by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > > >    by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > > >    by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > > >    by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > > >    by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > >    by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > > > > > > ---
> > > > > > > > > > >  src/gatt-client.c | 24 ++++++++++++------------
> > > > > > > > > > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > > > > > > --- a/src/gatt-client.c
> > > > > > > > > > > +++ b/src/gatt-client.c
> > > > > > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > > > > > >         { }
> > > > > > > > > > >  };
> > > > > > > > > > >
> > > > > > > > > > > +static void remove_client(void *data)
> > > > > > > > > > > +{
> > > > > > > > > > > +       struct notify_client *ntfy_client = data;
> > > > > > > > > > > +       struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > > > +
> > > > > > > > > > > +       queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > > > +
> > > > > > > > > > > +       notify_client_unref(ntfy_client);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > >  static void characteristic_free(void *data)
> > > > > > > > > > >  {
> > > > > > > > > > >         struct characteristic *chrc = data;
> > > > > > > > > > >
> > > > > > > > > > >         /* List should be empty here */
> > > > > > > > > > >         queue_destroy(chrc->descs, NULL);
> > > > > > > > > > > -       queue_destroy(chrc->notify_clients, NULL);
> > > > > > > > > > >
> > > > > > > > > > >         if (chrc->write_io) {
> > > > > > > > > > >                 queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > > > > > >                 pipe_io_destroy(chrc->notify_io);
> > > > > > > > > > >         }
> > > > > > > > > > >
> > > > > > > > > > > +       queue_destroy(chrc->notify_clients, remove_client);
> > > > > > > > > > > +
> > > > > > > > > > >         g_free(chrc->path);
> > > > > > > > > > >         free(chrc);
> > > > > > > > > > >  }
> > > > > > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > > > > > >         return chrc;
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > > -static void remove_client(void *data)
> > > > > > > > > > > -{
> > > > > > > > > > > -       struct notify_client *ntfy_client = data;
> > > > > > > > > > > -       struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > > > -
> > > > > > > > > > > -       queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > > > -
> > > > > > > > > > > -       notify_client_unref(ntfy_client);
> > > > > > > > > > > -}
> > > > > > > > > > > -
> > > > > > > > > > >  static void unregister_characteristic(void *data)
> > > > > > > > > > >  {
> > > > > > > > > > >         struct characteristic *chrc = data;
> > > > > > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > > > > > >         if (chrc->write_op)
> > > > > > > > > > >                 bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > > > > > > >
> > > > > > > > > > > -       queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > > > > > >         queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > > > > > > >
> > > > > > > > > > >         g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > > > > > > --
> > > > > > > > > > > 2.17.2
> > >
> > > Applied.
> > >
> > > --
> > > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-19 15:43 Luiz Augusto von Dentz
2018-11-19 15:43 ` [PATCH BlueZ 2/5] doc/gatt-api: Restrict supported file descriptors Luiz Augusto von Dentz
2018-11-19 15:43 ` [PATCH BlueZ 3/5] gatt: Switch from pipe2 to sockepair for Acquire* Luiz Augusto von Dentz
2018-11-19 15:43 ` [PATCH BlueZ 4/5] client: Switch from write to sendmsg " Luiz Augusto von Dentz
2018-11-19 15:43 ` [PATCH BlueZ 5/5] meshctl: " Luiz Augusto von Dentz
2018-11-20  8:50 ` [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting Luiz Augusto von Dentz
2018-11-20  9:30   ` Gal Ben Haim
2018-11-20 10:35     ` Luiz Augusto von Dentz
2018-11-20 12:35       ` Gal Ben Haim
2018-11-20 13:59         ` Luiz Augusto von Dentz
2018-11-20 18:31           ` Gal Ben Haim
2018-11-21  7:15             ` Gal Ben Haim
2018-11-21  9:15               ` Luiz Augusto von Dentz
2018-12-15  6:45                 ` Gal Ben Haim
2018-12-22 18:51                   ` Gal Ben Haim
2019-01-01 19:27                     ` Luiz Augusto von Dentz [this message]
     [not found]                       ` <CAHotPr9pgFt6URGN4ZUqngtQAijsGy_7uESibAAnmLSdagv6pA@mail.gmail.com>
2019-02-05  7:11                         ` Gal Ben Haim

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABBYNZ+sPBv8QdMNp7-fTD_d9g2atNWka1whhD1iUas4g1JWKA@mail.gmail.com \
    --to=luiz.dentz@gmail.com \
    --cc=gbenhaim@augury.com \
    --cc=linux-bluetooth@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox