diff mbox series

[ovs-dev] tests: Handle endianness in netlink policy test

Message ID 20211118082622.4147433-1-frode.nordahl@canonical.com
State Superseded
Headers show
Series [ovs-dev] tests: Handle endianness in netlink policy test | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Frode Nordahl Nov. 18, 2021, 8:26 a.m. UTC 
The netlink policy unit test contains test fixture data that is
subject to endianness and currently fails on big endian systems.

Add helper that ensures correct byte order for the affected data.

Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.")
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
---
 tests/test-netlink-policy.c | 58 +++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 21 deletions(-)

Comments

Michael Santana Nov. 18, 2021, 5:22 p.m. UTC | #1 
On 11/18/21 3:26 AM, Frode Nordahl wrote:
> The netlink policy unit test contains test fixture data that is
> subject to endianness and currently fails on big endian systems.
> 
> Add helper that ensures correct byte order for the affected data.
> 
> Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.")
> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> ---
>   tests/test-netlink-policy.c | 58 +++++++++++++++++++++++--------------
>   1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c
> index 5f2bf7101..ff00b3f04 100644
> --- a/tests/test-netlink-policy.c
> +++ b/tests/test-netlink-policy.c
> @@ -32,6 +32,22 @@ _nla_len(const struct nlattr *nla) {
>       return nla->nla_len - NLA_HDRLEN;
>   }
>   
> +/* The header part of the test fixture data is subject to endianness.  This
> + * helper makes sure they are put into the buffer in the right order. */
> +static void
> +prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) {
> +    uint16_t hword;
> +
> +    ovs_assert(size >= 4);
> +
> +    ofpbuf_init(buf, size);
> +    hword = data[0] | data[1] << 8;
> +    ofpbuf_put(buf, &hword, sizeof(hword));
> +    hword = data[2] | data[3] << 8;
> +    ofpbuf_put(buf, &hword, sizeof(hword));
> +    ofpbuf_put(buf, &data[4], size - sizeof(hword) * 2);
Quick question about this. Wouldnt data[4] fail on run time on the 
fixture_nl_data_policy_short case? Maybe I am misunderstanding something 
so clarify it for me if thats the case.

I see that fixture_nl_data_policy_short is declared as an array of 
uint8_t which contains 4 elements. Trying to access a (non-existing) 
fifth element (i.e. by data[4]) wouldnt create a run time error?
> +}
> +
>   static void
>   test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
>       struct nl_policy policy[] = {
> @@ -62,37 +78,37 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
>           /* valid policy but data neither eth_addr nor ib_addr */
>           0x0b, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a, 0x00,
>       };
> -    struct ofpbuf *buf;
> +    struct ofpbuf buf;
>   
>       /* confirm policy fails with too short data */
> -    buf = ofpbuf_clone_data(&fixture_nl_data_policy_short,
> -                            sizeof(fixture_nl_data_policy_short));
> -    ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> -    ofpbuf_delete(buf);
> +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_short,
> +                           sizeof(fixture_nl_data_policy_short));
> +    ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> +    ofpbuf_uninit(&buf);
>       memset(&attrs, 0, sizeof(*attrs));
>   
>       /* confirm policy fails with too long data */
> -    buf = ofpbuf_clone_data(&fixture_nl_data_policy_long,
> -                            sizeof(fixture_nl_data_policy_long));
> -    ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> -    ofpbuf_delete(buf);
> +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_long,
> +                           sizeof(fixture_nl_data_policy_long));
> +    ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> +    ofpbuf_uninit(&buf);
>       memset(&attrs, 0, sizeof(*attrs));
>   
>       /* confirm policy passes and interpret valid ethernet lladdr */
> -    buf = ofpbuf_clone_data(&fixture_nl_data_eth,
> -                            sizeof(fixture_nl_data_eth));
> -    ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_eth,
> +                           sizeof(fixture_nl_data_eth));
> +    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
>       ovs_assert((_nla_len(attrs[42]) == sizeof(struct eth_addr)));
>       struct eth_addr eth_expect = ETH_ADDR_C(00,53,00,00,00,2a);
>       struct eth_addr eth_parsed = nl_attr_get_eth_addr(attrs[42]);
>       ovs_assert((!memcmp(&eth_expect, &eth_parsed, sizeof(struct eth_addr))));
> -    ofpbuf_delete(buf);
> +    ofpbuf_uninit(&buf);
>       memset(&attrs, 0, sizeof(*attrs));
>   
>       /* confirm policy passes and interpret valid infiniband lladdr */
> -    buf = ofpbuf_clone_data(&fixture_nl_data_ib,
> -                            sizeof(fixture_nl_data_ib));
> -    ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_ib,
> +                           sizeof(fixture_nl_data_ib));
> +    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
>       ovs_assert((_nla_len(attrs[42]) == sizeof(struct ib_addr)));
>       struct ib_addr ib_expect = {
>               .ia = {
> @@ -102,18 +118,18 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
>       };
>       struct ib_addr ib_parsed = nl_attr_get_ib_addr(attrs[42]);
>       ovs_assert((!memcmp(&ib_expect, &ib_parsed, sizeof(struct eth_addr))));
> -    ofpbuf_delete(buf);
> +    ofpbuf_uninit(&buf);
>       memset(&attrs, 0, sizeof(*attrs));
>   
>       /* confirm we're able to detect invalid data that passes policy check, this
>        * can happen because the policy defines the data to be between the
>        * currently known lladdr sizes of 6 (ETH_ALEN) and 20 (INFINIBAND_ALEN) */
> -    buf = ofpbuf_clone_data(&fixture_nl_data_invalid,
> -                            sizeof(fixture_nl_data_invalid));
> -    ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_invalid,
> +                           sizeof(fixture_nl_data_invalid));
> +    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
>       ovs_assert(_nla_len(attrs[42]) != sizeof(struct eth_addr)
>                  && _nla_len(attrs[42]) != sizeof(struct ib_addr));
> -    ofpbuf_delete(buf);
> +    ofpbuf_uninit(&buf);
>       memset(&attrs, 0, sizeof(*attrs));
>   }
>   
>
Frode Nordahl Nov. 18, 2021, 5:56 p.m. UTC | #2 
tor. 18. nov. 2021, 18:22 skrev Michael Santana <msantana@redhat.com>:

>
>
> On 11/18/21 3:26 AM, Frode Nordahl wrote:
> > The netlink policy unit test contains test fixture data that is
> > subject to endianness and currently fails on big endian systems.
> >
> > Add helper that ensures correct byte order for the affected data.
> >
> > Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer
> address.")
> > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> > ---
> >   tests/test-netlink-policy.c | 58 +++++++++++++++++++++++--------------
> >   1 file changed, 37 insertions(+), 21 deletions(-)
> >
> > diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c
> > index 5f2bf7101..ff00b3f04 100644
> > --- a/tests/test-netlink-policy.c
> > +++ b/tests/test-netlink-policy.c
> > @@ -32,6 +32,22 @@ _nla_len(const struct nlattr *nla) {
> >       return nla->nla_len - NLA_HDRLEN;
> >   }
> >
> > +/* The header part of the test fixture data is subject to endianness.
> This
> > + * helper makes sure they are put into the buffer in the right order. */
> > +static void
> > +prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) {
> > +    uint16_t hword;
> > +
> > +    ovs_assert(size >= 4);
> > +
> > +    ofpbuf_init(buf, size);
> > +    hword = data[0] | data[1] << 8;
> > +    ofpbuf_put(buf, &hword, sizeof(hword));
> > +    hword = data[2] | data[3] << 8;
> > +    ofpbuf_put(buf, &hword, sizeof(hword));
> > +    ofpbuf_put(buf, &data[4], size - sizeof(hword) * 2);
> Quick question about this. Wouldnt data[4] fail on run time on the
> fixture_nl_data_policy_short case? Maybe I am misunderstanding something
> so clarify it for me if thats the case.
>
> I see that fixture_nl_data_policy_short is declared as an array of
> uint8_t which contains 4 elements. Trying to access a (non-existing)
> fifth element (i.e. by data[4]) wouldnt create a run time error?
>

Thank you for your review. In the case of a 4 element uint8 array a size of
4 will be passed in, which in consequence will pass 0 to ofpbuf_put which
will prevent memcpy from accessing the data. The test evidently passes, and
it did even pass tests with address sanitation enabled when I tested
locally. You caught me cutting corners for unit test helpers :)

I do agree with you that having code that accidentally can address
uninitialised memory is bad style regardless, so I'll send a V2. Thanks!

--
Frode Nordahl

> +}
> > +
> >   static void
> >   test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
> >       struct nl_policy policy[] = {
> > @@ -62,37 +78,37 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context
> *ctx OVS_UNUSED) {
> >           /* valid policy but data neither eth_addr nor ib_addr */
> >           0x0b, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a,
> 0x00,
> >       };
> > -    struct ofpbuf *buf;
> > +    struct ofpbuf buf;
> >
> >       /* confirm policy fails with too short data */
> > -    buf = ofpbuf_clone_data(&fixture_nl_data_policy_short,
> > -                            sizeof(fixture_nl_data_policy_short));
> > -    ovs_assert(!nl_policy_parse(buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > -    ofpbuf_delete(buf);
> > +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_short,
> > +                           sizeof(fixture_nl_data_policy_short));
> > +    ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > +    ofpbuf_uninit(&buf);
> >       memset(&attrs, 0, sizeof(*attrs));
> >
> >       /* confirm policy fails with too long data */
> > -    buf = ofpbuf_clone_data(&fixture_nl_data_policy_long,
> > -                            sizeof(fixture_nl_data_policy_long));
> > -    ovs_assert(!nl_policy_parse(buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > -    ofpbuf_delete(buf);
> > +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_long,
> > +                           sizeof(fixture_nl_data_policy_long));
> > +    ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > +    ofpbuf_uninit(&buf);
> >       memset(&attrs, 0, sizeof(*attrs));
> >
> >       /* confirm policy passes and interpret valid ethernet lladdr */
> > -    buf = ofpbuf_clone_data(&fixture_nl_data_eth,
> > -                            sizeof(fixture_nl_data_eth));
> > -    ovs_assert(nl_policy_parse(buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_eth,
> > +                           sizeof(fixture_nl_data_eth));
> > +    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> >       ovs_assert((_nla_len(attrs[42]) == sizeof(struct eth_addr)));
> >       struct eth_addr eth_expect = ETH_ADDR_C(00,53,00,00,00,2a);
> >       struct eth_addr eth_parsed = nl_attr_get_eth_addr(attrs[42]);
> >       ovs_assert((!memcmp(&eth_expect, &eth_parsed, sizeof(struct
> eth_addr))));
> > -    ofpbuf_delete(buf);
> > +    ofpbuf_uninit(&buf);
> >       memset(&attrs, 0, sizeof(*attrs));
> >
> >       /* confirm policy passes and interpret valid infiniband lladdr */
> > -    buf = ofpbuf_clone_data(&fixture_nl_data_ib,
> > -                            sizeof(fixture_nl_data_ib));
> > -    ovs_assert(nl_policy_parse(buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_ib,
> > +                           sizeof(fixture_nl_data_ib));
> > +    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> >       ovs_assert((_nla_len(attrs[42]) == sizeof(struct ib_addr)));
> >       struct ib_addr ib_expect = {
> >               .ia = {
> > @@ -102,18 +118,18 @@ test_nl_policy_parse_ll_addr(struct
> ovs_cmdl_context *ctx OVS_UNUSED) {
> >       };
> >       struct ib_addr ib_parsed = nl_attr_get_ib_addr(attrs[42]);
> >       ovs_assert((!memcmp(&ib_expect, &ib_parsed, sizeof(struct
> eth_addr))));
> > -    ofpbuf_delete(buf);
> > +    ofpbuf_uninit(&buf);
> >       memset(&attrs, 0, sizeof(*attrs));
> >
> >       /* confirm we're able to detect invalid data that passes policy
> check, this
> >        * can happen because the policy defines the data to be between the
> >        * currently known lladdr sizes of 6 (ETH_ALEN) and 20
> (INFINIBAND_ALEN) */
> > -    buf = ofpbuf_clone_data(&fixture_nl_data_invalid,
> > -                            sizeof(fixture_nl_data_invalid));
> > -    ovs_assert(nl_policy_parse(buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> > +    prepare_ofpbuf_fixture(&buf, fixture_nl_data_invalid,
> > +                           sizeof(fixture_nl_data_invalid));
> > +    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs,
> ARRAY_SIZE(policy)));
> >       ovs_assert(_nla_len(attrs[42]) != sizeof(struct eth_addr)
> >                  && _nla_len(attrs[42]) != sizeof(struct ib_addr));
> > -    ofpbuf_delete(buf);
> > +    ofpbuf_uninit(&buf);
> >       memset(&attrs, 0, sizeof(*attrs));
> >   }
> >
> >
>
>
diff mbox series

Patch

diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c
index 5f2bf7101..ff00b3f04 100644
--- a/tests/test-netlink-policy.c
+++ b/tests/test-netlink-policy.c
@@ -32,6 +32,22 @@  _nla_len(const struct nlattr *nla) {
     return nla->nla_len - NLA_HDRLEN;
 }
 
+/* The header part of the test fixture data is subject to endianness.  This
+ * helper makes sure they are put into the buffer in the right order. */
+static void
+prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) {
+    uint16_t hword;
+
+    ovs_assert(size >= 4);
+
+    ofpbuf_init(buf, size);
+    hword = data[0] | data[1] << 8;
+    ofpbuf_put(buf, &hword, sizeof(hword));
+    hword = data[2] | data[3] << 8;
+    ofpbuf_put(buf, &hword, sizeof(hword));
+    ofpbuf_put(buf, &data[4], size - sizeof(hword) * 2);
+}
+
 static void
 test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
     struct nl_policy policy[] = {
@@ -62,37 +78,37 @@  test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
         /* valid policy but data neither eth_addr nor ib_addr */
         0x0b, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a, 0x00,
     };
-    struct ofpbuf *buf;
+    struct ofpbuf buf;
 
     /* confirm policy fails with too short data */
-    buf = ofpbuf_clone_data(&fixture_nl_data_policy_short,
-                            sizeof(fixture_nl_data_policy_short));
-    ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
-    ofpbuf_delete(buf);
+    prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_short,
+                           sizeof(fixture_nl_data_policy_short));
+    ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
+    ofpbuf_uninit(&buf);
     memset(&attrs, 0, sizeof(*attrs));
 
     /* confirm policy fails with too long data */
-    buf = ofpbuf_clone_data(&fixture_nl_data_policy_long,
-                            sizeof(fixture_nl_data_policy_long));
-    ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
-    ofpbuf_delete(buf);
+    prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_long,
+                           sizeof(fixture_nl_data_policy_long));
+    ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
+    ofpbuf_uninit(&buf);
     memset(&attrs, 0, sizeof(*attrs));
 
     /* confirm policy passes and interpret valid ethernet lladdr */
-    buf = ofpbuf_clone_data(&fixture_nl_data_eth,
-                            sizeof(fixture_nl_data_eth));
-    ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
+    prepare_ofpbuf_fixture(&buf, fixture_nl_data_eth,
+                           sizeof(fixture_nl_data_eth));
+    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
     ovs_assert((_nla_len(attrs[42]) == sizeof(struct eth_addr)));
     struct eth_addr eth_expect = ETH_ADDR_C(00,53,00,00,00,2a);
     struct eth_addr eth_parsed = nl_attr_get_eth_addr(attrs[42]);
     ovs_assert((!memcmp(&eth_expect, &eth_parsed, sizeof(struct eth_addr))));
-    ofpbuf_delete(buf);
+    ofpbuf_uninit(&buf);
     memset(&attrs, 0, sizeof(*attrs));
 
     /* confirm policy passes and interpret valid infiniband lladdr */
-    buf = ofpbuf_clone_data(&fixture_nl_data_ib,
-                            sizeof(fixture_nl_data_ib));
-    ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
+    prepare_ofpbuf_fixture(&buf, fixture_nl_data_ib,
+                           sizeof(fixture_nl_data_ib));
+    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
     ovs_assert((_nla_len(attrs[42]) == sizeof(struct ib_addr)));
     struct ib_addr ib_expect = {
             .ia = {
@@ -102,18 +118,18 @@  test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
     };
     struct ib_addr ib_parsed = nl_attr_get_ib_addr(attrs[42]);
     ovs_assert((!memcmp(&ib_expect, &ib_parsed, sizeof(struct eth_addr))));
-    ofpbuf_delete(buf);
+    ofpbuf_uninit(&buf);
     memset(&attrs, 0, sizeof(*attrs));
 
     /* confirm we're able to detect invalid data that passes policy check, this
      * can happen because the policy defines the data to be between the
      * currently known lladdr sizes of 6 (ETH_ALEN) and 20 (INFINIBAND_ALEN) */
-    buf = ofpbuf_clone_data(&fixture_nl_data_invalid,
-                            sizeof(fixture_nl_data_invalid));
-    ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
+    prepare_ofpbuf_fixture(&buf, fixture_nl_data_invalid,
+                           sizeof(fixture_nl_data_invalid));
+    ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy)));
     ovs_assert(_nla_len(attrs[42]) != sizeof(struct eth_addr)
                && _nla_len(attrs[42]) != sizeof(struct ib_addr));
-    ofpbuf_delete(buf);
+    ofpbuf_uninit(&buf);
     memset(&attrs, 0, sizeof(*attrs));
 }