Message ID | 20211118082622.4147433-1-frode.nordahl@canonical.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] tests: Handle endianness in netlink policy test | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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(ð_expect, ð_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)); > } > >
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(ð_expect, ð_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 --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(ð_expect, ð_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)); }
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(-)