diff mbox series

[BlueZ,1/2] tools/mesh-cfgclient: Prevent storing duplicate models

Message ID 20230316180759.147486-1-inga.stotland@gmail.com (mailing list archive)
State Superseded
Headers show
Series [BlueZ,1/2] tools/mesh-cfgclient: Prevent storing duplicate models | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Inga Stotland March 16, 2023, 6:07 p.m. UTC 
This fixes the situation when subsequent requests to get a node
composition result in appending element's model list with duplicate models.
This adds a check for a presence of a model on a element when attempting
to add a new model ID to a model list on this element.
---
 tools/mesh/mesh-db.c | 17 ++++++++++++-----
 tools/mesh/remote.c  | 12 ++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

Comments

bluez.test.bot@gmail.com March 16, 2023, 7:31 p.m. UTC | #1 
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=730934

---Test result---

Test Summary:
CheckPatch                    PASS      0.91 seconds
GitLint                       PASS      0.57 seconds
BuildEll                      PASS      28.31 seconds
BluezMake                     PASS      886.31 seconds
MakeCheck                     PASS      11.37 seconds
MakeDistcheck                 PASS      155.54 seconds
CheckValgrind                 PASS      263.36 seconds
CheckSmatch                   PASS      336.69 seconds
bluezmakeextell               PASS      100.41 seconds
IncrementalBuild              PASS      1489.30 seconds
ScanBuild                     PASS      1054.85 seconds



---
Regards,
Linux Bluetooth
Paul Menzel March 16, 2023, 8:53 p.m. UTC | #2 
Dear Inga,


Thank you for your patch.

Am 16.03.23 um 19:07 schrieb Inga Stotland:
> This fixes the situation when subsequent requests to get a node
> composition result in appending element's model list with duplicate models.
> This adds a check for a presence of a model on a element when attempting

a*n* element

> to add a new model ID to a model list on this element.

How can this issue be reproduced?

> ---
>   tools/mesh/mesh-db.c | 17 ++++++++++++-----
>   tools/mesh/remote.c  | 12 ++++++++++++
>   2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c
> index c0c05a29a..1d047691d 100644
> --- a/tools/mesh/mesh-db.c
> +++ b/tools/mesh/mesh-db.c
> @@ -1964,28 +1964,35 @@ bool mesh_db_node_set_composition(uint16_t unicast, uint8_t *data, uint16_t len)
>   
>   		while (len >= 2 && m--) {
>   			mod_id = l_get_le16(data);
> +			data += 2;
> +			len -= 2;
> +
> +			jobj = get_model(unicast, unicast + i, mod_id, false);
> +			if (jobj)
> +				continue;
>   
>   			jobj = init_model(mod_id);
>   			if (!jobj)
>   				goto fail;
>   
>   			json_object_array_add(jmods, jobj);
> -			data += 2;
> -			len -= 2;
>   		}
>   
>   		while (len >= 4 && v--) {
>   			mod_id = l_get_le16(data + 2);
>   			mod_id = l_get_le16(data) << 16 | mod_id;
> +			data += 4;
> +			len -= 4;
> +
> +			jobj = get_model(unicast, unicast + i, mod_id, true);
> +			if (jobj)
> +				continue;
>   
>   			jobj = init_vendor_model(mod_id);
>   			if (!jobj)
>   				goto fail;
>   
>   			json_object_array_add(jmods, jobj);
> -
> -			data += 4;
> -			len -= 4;
>   		}
>   
>   		i++;
> diff --git a/tools/mesh/remote.c b/tools/mesh/remote.c
> index cee711dec..ec1476ac9 100644
> --- a/tools/mesh/remote.c
> +++ b/tools/mesh/remote.c
> @@ -54,6 +54,14 @@ struct rejected_addr {
>   static struct l_queue *nodes;
>   static struct l_queue *reject_list;
>   
> +static bool match_mod_id(const void *a, const void *b)
> +{
> +	uint32_t id1 = L_PTR_TO_UINT(a);
> +	uint32_t id2 = L_PTR_TO_UINT(b);

Why not `unsigned int`?

> +
> +	return id1 == id2;
> +}
> +
>   static int compare_mod_id(const void *a, const void *b, void *user_data)
>   {
>   	uint32_t id1 = L_PTR_TO_UINT(a);
> @@ -227,6 +235,10 @@ bool remote_set_model(uint16_t unicast, uint8_t ele_idx, uint32_t mod_id,
>   	if (!vendor)
>   		mod_id = VENDOR_ID_MASK | mod_id;
>   
> +	if (l_queue_find(rmt->els[ele_idx], match_mod_id,
> +							L_UINT_TO_PTR(mod_id)))
> +		return true;
> +
>   	l_queue_insert(rmt->els[ele_idx], L_UINT_TO_PTR(mod_id),
>   							compare_mod_id, NULL);
>   


Kind regards,

Paul
Stotland, Inga March 16, 2023, 9:19 p.m. UTC | #3 
Hi Paul,

On Thu, 2023-03-16 at 21:53 +0100, Paul Menzel wrote:
> Dear Inga,
> 
> 
> Thank you for your patch.
> 
> Am 16.03.23 um 19:07 schrieb Inga Stotland:
> > This fixes the situation when subsequent requests to get a node
> > composition result in appending element's model list with duplicate
> > models.
> > This adds a check for a presence of a model on a element when
> > attempting
> 
> a*n* element

OK
> 

> > to add a new model ID to a model list on this element.
> 
> How can this issue be reproduced?

The simplest way to reproduce the issue is to call composition-get on
the local node (i.e., target address 0001) and then issue command list-
nodes. Without the fix, the model list grows with each subsequent call
to composition-get.

> 
> > ---
> >   tools/mesh/mesh-db.c | 17 ++++++++++++-----
> >   tools/mesh/remote.c  | 12 ++++++++++++
> >   2 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c
> > index c0c05a29a..1d047691d 100644
> > --- a/tools/mesh/mesh-db.c
> > +++ b/tools/mesh/mesh-db.c
> > @@ -1964,28 +1964,35 @@ bool mesh_db_node_set_composition(uint16_t
> > unicast, uint8_t *data, uint16_t len)
> >   
> >                 while (len >= 2 && m--) {
> >                         mod_id = l_get_le16(data);
> > +                       data += 2;
> > +                       len -= 2;
> > +
> > +                       jobj = get_model(unicast, unicast + i,
> > mod_id, false);
> > +                       if (jobj)
> > +                               continue;
> >   
> >                         jobj = init_model(mod_id);
> >                         if (!jobj)
> >                                 goto fail;
> >   
> >                         json_object_array_add(jmods, jobj);
> > -                       data += 2;
> > -                       len -= 2;
> >                 }
> >   
> >                 while (len >= 4 && v--) {
> >                         mod_id = l_get_le16(data + 2);
> >                         mod_id = l_get_le16(data) << 16 | mod_id;
> > +                       data += 4;
> > +                       len -= 4;
> > +
> > +                       jobj = get_model(unicast, unicast + i,
> > mod_id, true);
> > +                       if (jobj)
> > +                               continue;
> >   
> >                         jobj = init_vendor_model(mod_id);
> >                         if (!jobj)
> >                                 goto fail;
> >   
> >                         json_object_array_add(jmods, jobj);
> > -
> > -                       data += 4;
> > -                       len -= 4;
> >                 }
> >   
> >                 i++;
> > diff --git a/tools/mesh/remote.c b/tools/mesh/remote.c
> > index cee711dec..ec1476ac9 100644
> > --- a/tools/mesh/remote.c
> > +++ b/tools/mesh/remote.c
> > @@ -54,6 +54,14 @@ struct rejected_addr {
> >   static struct l_queue *nodes;
> >   static struct l_queue *reject_list;
> >   
> > +static bool match_mod_id(const void *a, const void *b)
> > +{
> > +       uint32_t id1 = L_PTR_TO_UINT(a);
> > +       uint32_t id2 = L_PTR_TO_UINT(b);
> 
> Why not `unsigned int`?

Not sure what exactly you're asking here. Model ID is 32 bit long.
Anyway, will change to straight comparison without potential type
conversion.

> 
> > +
> > +       return id1 == id2;
> > +}
> > +
> >   static int compare_mod_id(const void *a, const void *b, void
> > *user_data)
> >   {
> >         uint32_t id1 = L_PTR_TO_UINT(a);
> > @@ -227,6 +235,10 @@ bool remote_set_model(uint16_t unicast,
> > uint8_t ele_idx, uint32_t mod_id,
> >         if (!vendor)
> >                 mod_id = VENDOR_ID_MASK | mod_id;
> >   
> > +       if (l_queue_find(rmt->els[ele_idx], match_mod_id,
> > +                                                       L_UINT_TO_P
> > TR(mod_id)))
> > +               return true;
> > +
> >         l_queue_insert(rmt->els[ele_idx], L_UINT_TO_PTR(mod_id),
> >                                                         compare_mod
> > _id, NULL);
> >   
> 
> 
> Kind regards,
> 
> Paul

Best regards,
Inga
diff mbox series

Patch

diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c
index c0c05a29a..1d047691d 100644
--- a/tools/mesh/mesh-db.c
+++ b/tools/mesh/mesh-db.c
@@ -1964,28 +1964,35 @@  bool mesh_db_node_set_composition(uint16_t unicast, uint8_t *data, uint16_t len)
 
 		while (len >= 2 && m--) {
 			mod_id = l_get_le16(data);
+			data += 2;
+			len -= 2;
+
+			jobj = get_model(unicast, unicast + i, mod_id, false);
+			if (jobj)
+				continue;
 
 			jobj = init_model(mod_id);
 			if (!jobj)
 				goto fail;
 
 			json_object_array_add(jmods, jobj);
-			data += 2;
-			len -= 2;
 		}
 
 		while (len >= 4 && v--) {
 			mod_id = l_get_le16(data + 2);
 			mod_id = l_get_le16(data) << 16 | mod_id;
+			data += 4;
+			len -= 4;
+
+			jobj = get_model(unicast, unicast + i, mod_id, true);
+			if (jobj)
+				continue;
 
 			jobj = init_vendor_model(mod_id);
 			if (!jobj)
 				goto fail;
 
 			json_object_array_add(jmods, jobj);
-
-			data += 4;
-			len -= 4;
 		}
 
 		i++;
diff --git a/tools/mesh/remote.c b/tools/mesh/remote.c
index cee711dec..ec1476ac9 100644
--- a/tools/mesh/remote.c
+++ b/tools/mesh/remote.c
@@ -54,6 +54,14 @@  struct rejected_addr {
 static struct l_queue *nodes;
 static struct l_queue *reject_list;
 
+static bool match_mod_id(const void *a, const void *b)
+{
+	uint32_t id1 = L_PTR_TO_UINT(a);
+	uint32_t id2 = L_PTR_TO_UINT(b);
+
+	return id1 == id2;
+}
+
 static int compare_mod_id(const void *a, const void *b, void *user_data)
 {
 	uint32_t id1 = L_PTR_TO_UINT(a);
@@ -227,6 +235,10 @@  bool remote_set_model(uint16_t unicast, uint8_t ele_idx, uint32_t mod_id,
 	if (!vendor)
 		mod_id = VENDOR_ID_MASK | mod_id;
 
+	if (l_queue_find(rmt->els[ele_idx], match_mod_id,
+							L_UINT_TO_PTR(mod_id)))
+		return true;
+
 	l_queue_insert(rmt->els[ele_idx], L_UINT_TO_PTR(mod_id),
 							compare_mod_id, NULL);