2023-03-16 18:09:00

by Inga Stotland

[permalink] [raw]
Subject: [PATCH BlueZ 1/2] tools/mesh-cfgclient: Prevent storing duplicate models

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

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

--
2.39.2



2023-03-16 18:09:06

by Inga Stotland

[permalink] [raw]
Subject: [PATCH BlueZ 2/2] tools/mesh-cfgclient: Auto request own composition data

When attaching a local provisioner node, always request own
composition data to accommodate functional consolidation of
regular and remote provisioning mechanisms.
The knowledge of the own node composition is necessary for
provisioning initiation and self configuration.
---
tools/mesh-cfgclient.c | 9 +++++++--
tools/mesh/cfgcli.c | 15 +++++++++++++++
tools/mesh/cfgcli.h | 2 ++
tools/mesh/util.c | 2 ++
4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c
index 50be82bcf..6d2d34409 100644
--- a/tools/mesh-cfgclient.c
+++ b/tools/mesh-cfgclient.c
@@ -43,7 +43,8 @@

#define CFG_SRV_MODEL 0x0000
#define CFG_CLI_MODEL 0x0001
-#define RPR_SVR_MODEL 0xFFFF0004
+#define RPR_SVR_MODEL 0x0004
+#define RPR_CLI_MODEL 0x0005
#define PRV_BEACON_SVR 0x0008
#define PRV_BEACON_CLI 0x0009

@@ -775,6 +776,10 @@ static void attach_node_reply(struct l_dbus_proxy *proxy,
remote_clear_rejected_addresses(ivi);
}

+ /* Read own node composition */
+ if (!cfgcli_get_comp(0x0001, 128))
+ l_error("Failed to read own composition");
+
return;

fail:
@@ -863,7 +868,7 @@ static void scan_start(void *user_data, uint16_t dst, uint32_t model)
{
struct scan_data *data;

- if (model != RPR_SVR_MODEL)
+ if (model != (0xffff0000 | RPR_SVR_MODEL))
return;

data = l_malloc(sizeof(struct scan_data));
diff --git a/tools/mesh/cfgcli.c b/tools/mesh/cfgcli.c
index 4f6248e48..1a404af38 100644
--- a/tools/mesh/cfgcli.c
+++ b/tools/mesh/cfgcli.c
@@ -1081,6 +1081,21 @@ static void cmd_default(uint32_t opcode)
return bt_shell_noninteractive_quit(EXIT_SUCCESS);
}

+
+bool cfgcli_get_comp(uint16_t unicast, uint8_t page)
+{
+ uint16_t n;
+ uint8_t msg[32];
+
+ n = mesh_opcode_set(OP_DEV_COMP_GET, msg);
+
+ msg[n++] = page;
+
+ target = unicast;
+
+ return config_send(msg, n, OP_DEV_COMP_GET);
+}
+
static void cmd_composition_get(int argc, char *argv[])
{
uint16_t n;
diff --git a/tools/mesh/cfgcli.h b/tools/mesh/cfgcli.h
index 7281caa46..621dd0259 100644
--- a/tools/mesh/cfgcli.h
+++ b/tools/mesh/cfgcli.h
@@ -19,4 +19,6 @@ typedef void (*delete_remote_func_t) (uint16_t primary, uint8_t ele_cnt);

struct model_info *cfgcli_init(key_send_func_t key_func,
delete_remote_func_t del_node, void *user_data);
+
+bool cfgcli_get_comp(uint16_t unicast, uint8_t page);
void cfgcli_cleanup(void);
diff --git a/tools/mesh/util.c b/tools/mesh/util.c
index d8c47c0e9..dea496dbe 100644
--- a/tools/mesh/util.c
+++ b/tools/mesh/util.c
@@ -138,6 +138,8 @@ const char *sig_model_string(uint16_t sig_model_id)
case 0x0001: return "Configuration Client";
case 0x0002: return "Health Server";
case 0x0003: return "Health Client";
+ case 0x0004: return "Remote Provisioning Server";
+ case 0x0005: return "Remote Provisioning Client";
case 0x0008: return "Private Beacon Server";
case 0x0009: return "Private Beacon Client";
case 0x1000: return "Generic OnOff Server";
--
2.39.2


2023-03-16 19:31:16

by bluez.test.bot

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

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

2023-03-16 20:53:30

by Paul Menzel

[permalink] [raw]
Subject: Re: [PATCH BlueZ 1/2] tools/mesh-cfgclient: Prevent storing duplicate models

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

2023-03-16 21:19:26

by Stotland, Inga

[permalink] [raw]
Subject: Re: [PATCH BlueZ 1/2] tools/mesh-cfgclient: Prevent storing duplicate models

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