On Dec 17, 2013, at 12:16 AM, Alex Wang <al...@nicira.com> wrote: > Commit da546e0 (dpif: Allow execute to modify the packet.) uninitializes > the "dpif_upcall.packet" of "struct upcall" when dpif_recv() returns error. > Since the "struct upcall" is allocated via xmalloc, this will cause SEGFAULT > if dpif_recv() returns error before memset the memory to all zero. >
Alex, thanks for digging this up! So the problem is that in error cases the ofpbuf packet member of upcall has been uninitialized. The fix you propose works because all-zero ofpbuf can be safely uninitialized. We have not documented this behavior anywhere, so I’m not sure if we should rely on it. The alternative would be to properly initialize the packet member also for error returns (and document whose responsibility it is.). I’ll send a patch soon to this effect for you to review. Jarno > This commit fixes the bug by using xzalloc to allocate memory for "struct > upcall". > > Signed-off-by: Alex Wang <al...@nicira.com> > --- > lib/dpif-linux.c | 2 +- > ofproto/ofproto-dpif-upcall.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c > index 54d7f2a..d11b484 100644 > --- a/lib/dpif-linux.c > +++ b/lib/dpif-linux.c > @@ -1440,6 +1440,7 @@ parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall > *upcall, > struct ofpbuf b; > int type; > > + memset(upcall, 0, sizeof *upcall); > ofpbuf_use_const(&b, buf->data, buf->size); > > nlmsg = ofpbuf_try_pull(&b, sizeof *nlmsg); > @@ -1459,7 +1460,6 @@ parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall > *upcall, > return EINVAL; > } > > - memset(upcall, 0, sizeof *upcall); > upcall->type = type; > upcall->key = CONST_CAST(struct nlattr *, > nl_attr_get(a[OVS_PACKET_ATTR_KEY])); > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 53a9e82..2bc684d 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -527,7 +527,7 @@ recv_upcalls(struct udpif *udpif) > struct nlattr *nla; > int error; > > - upcall = xmalloc(sizeof *upcall); > + upcall = xzalloc(sizeof *upcall); > ofpbuf_use_stub(&upcall->upcall_buf, upcall->upcall_stub, > sizeof upcall->upcall_stub); > error = dpif_recv(udpif->dpif, &upcall->dpif_upcall, > -- > 1.7.9.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
