tor. 18. nov. 2021, 18:22 skrev Michael Santana <msant...@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.nord...@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)); > > } > > > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev