Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200507141055.Bluez.1.Ie4c027829af0f3ca2ba0e532706584a554a69d38@changeid>
In-Reply-To: <20200507141055.Bluez.1.Ie4c027829af0f3ca2ba0e532706584a554a69d38@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 7 May 2020 15:07:45 -0700
Message-ID: <CABBYNZ+DJqfK8OoDJmp+OuJ7y4ML+31jdWNRzsEb7O6CDv3dsg@mail.gmail.com>
Subject: Re: [Bluez PATCH] shared/gatt-db: Check for null pointers
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Alain Michaud <alainm@chromium.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Abhishek,

On Thu, May 7, 2020 at 2:11 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Make sure the attribute, service and service attributes are not null
> before accessing them.


For this type of problem I would recommend using valgrind to
backtrace, since this is not very readable we might not want to add it
to the git history, that said the actual problem seem to be related to
using gatt_db_attribute_get_service_data while the service is being
removed, in that case I guess it is fine to set the service to NULL
before removing it to indicate the it is in the process of being
removed, we could perhaps introduce a unit test for this type of error
so we can detect regressions with the likes of make check and CI when
that starts to use it.

> The problem was seen with the following stack trace:
>
>  0  bluetoothd!gatt_db_attribute_get_service_data [gatt-db.c : 1648 + 0x0]
>     rax = 0x0000000000000000   rdx = 0x00007ffce8cf70ec
>     rcx = 0x0000000000000000   rbx = 0x0000000000000000
>     rsi = 0x00007ffce8cf70ee   rdi = 0x00005a56611f05c0
>     rbp = 0x00007ffce8cf70d0   rsp = 0x00007ffce8cf70b0
>      r8 = 0x0000000000000000    r9 = 0x0000000000000050
>     r10 = 0x0000000000000073   r11 = 0x0000000000000246
>     r12 = 0x00005a56611cea10   r13 = 0x00005a56611efd90
>     r14 = 0x0000000000000000   r15 = 0x00005a565f3eed8d
>     rip = 0x00005a565f48147e
>     Found by: given as instruction pointer in context
>  1  bluetoothd!discovery_op_complete [gatt-client.c : 386 + 0x14]
>     rbx = 0x00005a56611e9d30   rbp = 0x00007ffce8cf7120
>     rsp = 0x00007ffce8cf70e0   r12 = 0x00005a56611cea10
>     r13 = 0x00005a56611efd90   r14 = 0x00007ffce8cf70ec
>     r15 = 0x00005a565f3eed8d   rip = 0x00005a565f47a6bc
>     Found by: call frame info
>  2  bluetoothd!discover_chrcs_cb [gatt-client.c : 1000 + 0xf]
>     rbx = 0x0000000000000000   rbp = 0x00007ffce8cf71d0
>     rsp = 0x00007ffce8cf7130   r12 = 0x000000000000000a
>     r13 = 0x00005a56611de920   r14 = 0x00005a56611cea10
>     r15 = 0x00007ffce8cf7188   rip = 0x00005a565f47b18a
>     Found by: call frame info
>  3  bluetoothd!discovery_op_complete [gatt-helpers.c : 628 + 0xc]
>     rbx = 0x00005a56611f0430   rbp = 0x00007ffce8cf71f0
>     rsp = 0x00007ffce8cf71e0   r12 = 0x00005a56611ea5a0
>     r13 = 0x00005a56611cd430   r14 = 0x00005a56611f0430
>     r15 = 0x00005a566119bc01   rip = 0x00005a565f47d60e
>     Found by: call frame info
>  4  bluetoothd!discover_chrcs_cb [gatt-helpers.c : 1250 + 0xe]
>     rbx = 0x00005a56611bf0f1   rbp = 0x00007ffce8cf7240
>     rsp = 0x00007ffce8cf7200   r12 = 0x00005a56611ea5a0
>     r13 = 0x00005a56611cd430   r14 = 0x00005a56611f0430
>     r15 = 0x00005a566119bc01   rip = 0x00005a565f47cc7a
>     Found by: call frame info
>
> ---
>
>  src/shared/gatt-db.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index b44f7b5e9..2432bdfd4 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -513,6 +513,7 @@ bool gatt_db_remove_service(struct gatt_db *db,
>                 return false;
>
>         service = attrib->service;
> +       attrib->service = NULL;
>
>         queue_remove(db->services, service);
>
> @@ -1605,7 +1606,7 @@ bool gatt_db_attribute_get_service_data(const struct gatt_db_attribute *attrib,
>         struct gatt_db_service *service;
>         struct gatt_db_attribute *decl;
>
> -       if (!attrib)
> +       if (!(attrib && attrib->service && attrib->service->attributes))
>                 return false;
>
>         service = attrib->service;
> --
> 2.26.2.645.ge9eca65c58-goog
>


-- 
Luiz Augusto von Dentz