[ovs-dev] [PATCH] tests: Handle endianness in netlink policy test
Frode Nordahl
frode.nordahl at canonical.com
Thu Nov 18 17:56:21 UTC 2021
tor. 18. nov. 2021, 18:22 skrev Michael Santana <msantana at 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 at 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));
> > }
> >
> >
>
>
More information about the dev
mailing list