Thanks. I applied this to master and branch-2.9.
On Thu, May 10, 2018 at 09:13:41AM +0000, Anju Thomas wrote: > Hi Ben, > > Yes. This patch certainly looks more cleaner and better to me . > > Thanks > Anju > -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Thursday, May 10, 2018 2:33 AM > To: Anju Thomas <[email protected]> > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH v1] Fix crash when processing malformed Bundle > Add message in OVS > > On Mon, May 07, 2018 at 10:58:06PM +0530, Anju Thomas wrote: > > When an OpenFlow Bundle Add message is received, a bundle entry is > > created and the OpenFlow message embedded in the bundle add message is > > processed. If any error is encountered while processing the embedded > > message, the bundle entry is freed. The bundle entry free function > > assumes that the entry has been populated with a properly formatted > > OpenFlow message and performs some message specific cleanup actions . > > This assumption does not hold true in the error case and OVS crashes > > when performing the cleanup. > > > > The fix is in case of errors, simply free the bundle entry without > > attempting to perform any embedded message cleanup > > > > Signed-off-by: Anju Thomas <[email protected]> > > Thanks for the fix. > > It's not really nice to have multiple levels of initialized-ness. What do > you think of this fix instead? > > Thanks, > > Ben. > > --8<--------------------------cut here-------------------------->8-- > > From: Anju Thomas <[email protected]> > Date: Mon, 7 May 2018 22:58:06 +0530 > Subject: [PATCH] Fix crash when processing malformed Bundle Add message in OVS > > When an OpenFlow Bundle Add message is received, a bundle entry is created > and the OpenFlow message embedded in the bundle add message is processed. If > any error is encountered while processing the embedded message, the bundle > entry is freed. The bundle entry free function assumes that the entry has > been populated with a properly formatted OpenFlow message and performs some > message specific cleanup actions . > This assumption does not hold true in the error case and OVS crashes when > performing the cleanup. > > The fix is in case of errors, simply free the bundle entry without attempting > to perform any embedded message cleanup > > Signed-off-by: Anju Thomas <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > ofproto/bundles.h | 13 ------------- > ofproto/ofproto.c | 21 +++++++++++++-------- > 2 files changed, 13 insertions(+), 21 deletions(-) > > diff --git a/ofproto/bundles.h b/ofproto/bundles.h index > 806e853d6883..1164fddd8702 100644 > --- a/ofproto/bundles.h > +++ b/ofproto/bundles.h > @@ -58,8 +58,6 @@ struct ofp_bundle { > struct ovs_list msg_list; /* List of 'struct bundle_message's */ > }; > > -static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc( > - enum ofptype type, const struct ofp_header *oh); > static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *); > > enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags, @@ > -72,17 +70,6 @@ enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t > id, > > void ofp_bundle_remove__(struct ofconn *, struct ofp_bundle *); > > > -static inline struct ofp_bundle_entry * -ofp_bundle_entry_alloc(enum ofptype > type, const struct ofp_header *oh) -{ > - struct ofp_bundle_entry *entry = xmalloc(sizeof *entry); > - > - entry->type = type; > - entry->msg = xmemdup(oh, ntohs(oh->length)); > - > - return entry; > -} > - > static inline void > ofp_bundle_entry_free(struct ofp_bundle_entry *entry) { diff --git > a/ofproto/ofproto.c b/ofproto/ofproto.c index 36f4c0b16d48..565a45db1507 > 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -7899,7 +7899,6 @@ handle_bundle_add(struct ofconn *ofconn, const struct > ofp_header *oh) > struct ofproto *ofproto = ofconn_get_ofproto(ofconn); > enum ofperr error; > struct ofputil_bundle_add_msg badd; > - struct ofp_bundle_entry *bmsg; > enum ofptype type; > > error = reject_slave_controller(ofconn); @@ -7912,7 +7911,8 @@ > handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) > return error; > } > > - bmsg = ofp_bundle_entry_alloc(type, badd.msg); > + /* Allocate bundle entry and decode the embedded message. */ > + struct ofp_bundle_entry *bmsg = xmalloc(sizeof *bmsg); > > struct ofpbuf ofpacts; > uint64_t ofpacts_stub[1024 / 8]; > @@ -7951,18 +7951,23 @@ handle_bundle_add(struct ofconn *ofconn, const struct > ofp_header *oh) > } else { > OVS_NOT_REACHED(); > } > - > ofpbuf_uninit(&ofpacts); > - > - if (!error) { > - error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, > - bmsg, oh); > + if (error) { > + free(bmsg); > + return error; > } > > + /* Now that the embedded message has been successfully decoded, finish up > + * initializing the bundle entry. */ > + bmsg->type = type; > + bmsg->msg = xmemdup(oh, ntohs(oh->length)); > + > + /* Add bundle entry to bundle. */ > + error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, > + bmsg, oh); > if (error) { > ofp_bundle_entry_free(bmsg); > } > - > return error; > } > > -- > 2.16.1 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
