Applied to master and branch-2.4, Thanks, Alex Wang,
On Mon, Jul 20, 2015 at 9:58 AM, Alex Wang <al...@nicira.com> wrote: > Thx for the review, I'll adopt the comment~ > > On Mon, Jul 20, 2015 at 9:47 AM, Ben Pfaff <b...@nicira.com> wrote: > >> On Mon, Jul 20, 2015 at 08:50:39AM -0700, Alex Wang wrote: >> > On Mon, Jul 20, 2015 at 8:17 AM, Ben Pfaff <b...@nicira.com> wrote: >> > >> > > On Mon, Jul 20, 2015 at 01:22:32AM -0700, Alex Wang wrote: >> > > > Commit 6fd6ed7 (ofpbuf: Simplify ofpbuf API.) introduced the >> > > > 'header' and 'msg' pointers to 'struct ofpbuf'. However, we >> > > > forget to update the 'msg' pointer when resizing ofpbuf. >> > > > >> > > > This bug could cause serious issue. For example, in the function >> > > > ofputil_encode_nx_packet_in(), the 'msg' pointer is populated in >> > > > ofpraw_alloc_xid() when creating the ofpbuf . Later, the ofpbuf >> > > > memory can be reallocated due to the writing to the ofpbuf. >> > > > However, since the 'msg' pointer is not updated, the later use of >> > > > the 'ofpbuf->msg' will end up writing to either free'ed memory or >> > > > memory allocated for other struct. >> > > > >> > > > This commit fixes the bug by always updating the 'header' and >> > > > 'msg' pointers when the ofpbuf is resized. Also, a simple test >> > > > is added. >> > > > >> > > > Signed-off-by: Alex Wang <al...@nicira.com> >> > > >> > > Good catch! >> > > >> > > I don't understand the new comment on ofpbuf_trim(). >> ofpbuf_resize__() >> > > will adjust the pointers automatically, won't it? >> > > >> > >> > There is one corner case I tested, assume we 'b = ofpbuf_new(100)' >> first, >> > and then make 'b->header = b->base + 10', 'b->msg = b->base + 50', >> without >> > actually putting anything to the buffer yet. >> > >> > Then, calling 'ofpbuf_trim(b)' will trim 'b' to an empty ofpbuf but with >> > 'b->header' and 'b->msg' pointing invalid memory addresses. Dont think >> I >> > can find any real implication. What do youthink? >> >> That case makes sense. Maybe the comment could be more specific: >> >> * Updates 'b->header' and 'b->msg' so that they point to the same >> locations in >> * the data. (If they pointed into the tailroom or headroom then they >> become >> * invalid.) >> > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
