> On Jan 5, 2017, at 4:48 PM, Ben Pfaff <[email protected]> wrote: > > On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote: >> >>> On Jan 4, 2017, at 11:03 PM, Ben Pfaff <[email protected]> wrote: >>> >>> On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote: >>>> >>>>> On Dec 21, 2016, at 2:36 PM, Ben Pfaff <[email protected]> wrote: >>>>> >>>>> On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote: >>>>> I'd be more comfortable if nx_stack_pop() had assertions to check for >>>>> underflow. >>>> >>>> I don’t think OVS should assert fail if controller issues one pop >>>> too many? Do you mean that current users of nx_stack_pop() do not >>>> check for NULL return? I had a look and think that setting “*bytes” >>>> to zero when returning NULL should be enough for all users. >>> >>> It appears to me that if stack->size is greater than 0 but less than the >>> number of bytes indicated by its last byte, then it will corrupt the >>> ofpbuf size and that this will later cause some kind of failure that >>> will be harder to debug than an assertion failure. >>> >> >> OK, now i got it. This is just to guard against (future) bugs in OVS itself. > > Yes. > >>>>> In ofputil_decode_packet_in_private(), it's probably worth checking the >>>>> format of the stack we pull from the payload, since a badly formatted >>>>> stack can segfault us (if we leave out assertions) or assert-fail us (if >>>>> we include them). >>>>> >>>> >>>> What do you mean with badly formatted stack? Zero-sized property? IMO >>>> even that would be properly pushed and popped from the stack, storing >>>> only the length (of zero) in the stack. >>> >>> I mean that if the property contains, for example, a single byte with >>> value 0xff, then it's badly formatted because we can pop off a length >>> (255) but then popping off that number of bytes will underflow. >> >> I did not change the encoding of the stack as properties, so each >> value in the stack is still encoded as a separate property, where the >> (aligned) value length is used as the property length. > > I guess I forgot that. > > Thanks, that's fine then. > >> ofpprop_pull() does the length checking for the properties and the >> current code in ofputil_decode_packet_in_private() assert fails on any >> error, which is not good, as a controller bug would crash OVS? > > That's bad. Maybe the fix is as simple as this, though. > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index 156d8d2..421b9d7 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 > Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 > Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -4061,7 +4061,9 @@ ofputil_decode_packet_in_private(const struct > ofp_header *oh, bool loose, > uint64_t type; > > error = ofpprop_pull(&continuation, &payload, &type); > - ovs_assert(!error); > + if (error) { > + break; > + } > > switch (type) { > case NXCPT_BRIDGE: > @@ -4124,7 +4126,7 @@ ofputil_decode_packet_in_private(const struct > ofp_header *oh, bool loose, > ofputil_packet_in_private_destroy(pin); > } > > - return 0; > + return error; > } > > /* Frees data in 'pin' that is dynamically allocated by >
I folded this in to v3. Jarno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
