[Bluez PATCH v1] gatt-db: fix service in range check

[Bluez PATCH v1] gatt-db: fix service in range check

[Howard Chung]

From: Yun-Hao Chung <howardchung@chromium.org>

If foreach_data->start < svc_start < foreach_data->end < svc_end,
foreach_in_range runs foreach_service_in_range to this service.

This patch fix the above bug.

Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

 src/shared/gatt-db.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 8bff4d37aaa2..38d93f273a9e 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1349,7 +1349,7 @@ static void foreach_in_range(void *data, void *user_data)
 
 	if (!foreach_data->attr) {
 		if (svc_start < foreach_data->start ||
-					svc_start > foreach_data->end)
+						svc_end > foreach_data->end)
 			return;
 		return foreach_service_in_range(data, user_data);
 	}
-- 
2.32.0.432.gabb21c7263-goog

RE: [Bluez,v1] gatt-db: fix service in range check

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=521971

---Test result---

Test Summary:
CheckPatch                    PASS      0.37 seconds
GitLint                       PASS      0.14 seconds
Prep - Setup ELL              PASS      48.34 seconds
Build - Prep                  PASS      0.13 seconds
Build - Configure             PASS      8.49 seconds
Build - Make                  PASS      224.26 seconds
Make Check                    PASS      9.09 seconds
Make Distcheck                PASS      255.44 seconds
Build w/ext ELL - Configure   PASS      8.56 seconds
Build w/ext ELL - Make        PASS      201.55 seconds

Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf

##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - PASS
Desc: Build the BlueZ source tree

##############################
Test: Make Check - PASS
Desc: Run 'make check'

##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - PASS
Desc: Build BlueZ source with '--enable-external-ell' configuration



---
Regards,
Linux Bluetooth

Re: [Bluez PATCH v1] gatt-db: fix service in range check

[Luiz Augusto von Dentz]

Hi Howard,

On Tue, Jul 27, 2021 at 4:53 AM Howard Chung <howardchung@google.com> wrote:
>
> From: Yun-Hao Chung <howardchung@chromium.org>
>
> If foreach_data->start < svc_start < foreach_data->end < svc_end,
> foreach_in_range runs foreach_service_in_range to this service.
>
> This patch fix the above bug.
>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
>
>  src/shared/gatt-db.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 8bff4d37aaa2..38d93f273a9e 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -1349,7 +1349,7 @@ static void foreach_in_range(void *data, void *user_data)
>
>         if (!foreach_data->attr) {
>                 if (svc_start < foreach_data->start ||
> -                                       svc_start > foreach_data->end)
> +                                               svc_end > foreach_data->end)
>                         return;

Actually if I recall this check is correct, we want to iterate to
every service within the range of start to end, if the range is lets
say 1-9 and the service is 1-14 is still within the range since it
matches the range of 1-9, or perhaps you want to explain why that
would be a problem since that is not explained in the patch
description, if we do switch to strict matching it is probably worth
documenting that we would only match if the entire range of services
is within the range, anyway I would still keep svc_start >
foreach_data->end since that would stop iterating early instead of
continuing to all the list.

>                 return foreach_service_in_range(data, user_data);
>         }
> --
> 2.32.0.432.gabb21c7263-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v1] gatt-db: fix service in range check

[Yun-hao Chung]

I see. To be honest, I didn't encounter any issues with this function.
I was trying to solve another problem and found this might be a typo.
Thanks for the explanation.

On Wed, Jul 28, 2021 at 6:18 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Tue, Jul 27, 2021 at 4:53 AM Howard Chung <howardchung@google.com> wrote:
> >
> > From: Yun-Hao Chung <howardchung@chromium.org>
> >
> > If foreach_data->start < svc_start < foreach_data->end < svc_end,
> > foreach_in_range runs foreach_service_in_range to this service.
> >
> > This patch fix the above bug.
> >
> > Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> > ---
> >
> >  src/shared/gatt-db.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> > index 8bff4d37aaa2..38d93f273a9e 100644
> > --- a/src/shared/gatt-db.c
> > +++ b/src/shared/gatt-db.c
> > @@ -1349,7 +1349,7 @@ static void foreach_in_range(void *data, void *user_data)
> >
> >         if (!foreach_data->attr) {
> >                 if (svc_start < foreach_data->start ||
> > -                                       svc_start > foreach_data->end)
> > +                                               svc_end > foreach_data->end)
> >                         return;
>
> Actually if I recall this check is correct, we want to iterate to
> every service within the range of start to end, if the range is lets
> say 1-9 and the service is 1-14 is still within the range since it
> matches the range of 1-9, or perhaps you want to explain why that
> would be a problem since that is not explained in the patch
> description, if we do switch to strict matching it is probably worth
> documenting that we would only match if the entire range of services
> is within the range, anyway I would still keep svc_start >
> foreach_data->end since that would stop iterating early instead of
> continuing to all the list.
>
> >                 return foreach_service_in_range(data, user_data);
> >         }
> > --
> > 2.32.0.432.gabb21c7263-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Re: [Bluez PATCH v1] gatt-db: fix service in range check

[Yun-hao Chung]

> > >                 if (svc_start < foreach_data->start ||
> > >                                       svc_start > foreach_data->end)
> > >                         return;
After discussing with Archie, if we understand you correctly, we think
this early return can be removed.
Let's say the searched range is 5-9 and the service is 1-14, then we
should consider the service is in range.
If we want to keep the early return, svc_start > foreach_data->end is
already checked a few lines before, so this check is redundant.

Re: [Bluez PATCH v1] gatt-db: fix service in range check

[Luiz Augusto von Dentz]

Hi Howard,

On Tue, Jul 27, 2021 at 7:51 PM Yun-hao Chung <howardchung@google.com> wrote:
>
> > > >                 if (svc_start < foreach_data->start ||
> > > >                                       svc_start > foreach_data->end)
> > > >                         return;
> After discussing with Archie, if we understand you correctly, we think
> this early return can be removed.
> Let's say the searched range is 5-9 and the service is 1-14, then we
> should consider the service is in range.
> If we want to keep the early return, svc_start > foreach_data->end is
> already checked a few lines before, so this check is redundant.

Yep, that seems we have already had the same check so Im fine dropping
the check.

-- 
Luiz Augusto von Dentz