| Message ID | 1456993371-9902-1-git-send-email-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 3 March 2016 at 00:22, Joe Stringer <joe@ovn.org> wrote: > Add a test which causes internal reallocation of the ofpacts buffer, > followed by a large bundle action which should cause a subsequent > reallocation while decoding slave ports. Running this test under > valgrind reveals the issue below, which is fixed in the following > commit. > > Invalid read of size 4 > at 0x4CED87: decode_bundle (ofp-actions.c:1253) > by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) > by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) > by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) > by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) > by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) > by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) > by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) > by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) > by 0x4F0D98: ofp_to_string (ofp-print.c:3453) > by 0x5486B3: do_recv (vconn.c:644) > by 0x548498: vconn_recv (vconn.c:598) > by 0x524582: rconn_recv (rconn.c:703) > by 0x45DA61: ofconn_run (connmgr.c:1370) > by 0x45B3B4: connmgr_run (connmgr.c:323) > by 0x41D1E8: ofproto_run (ofproto.c:1762) > by 0x40CEE0: bridge_run__ (bridge.c:2885) > by 0x40D093: bridge_run (bridge.c:2940) > by 0x412F7E: main (ovs-vswitchd.c:120) > Address 0x66aa460 is 1,152 bytes inside a block of size 1,184 free'd > at 0x4C2AF2E: realloc (vg_replace_malloc.c:692) > by 0x543D27: xrealloc (util.c:123) > by 0x5089EF: ofpbuf_resize__ (ofpbuf.c:243) > by 0x508B81: ofpbuf_prealloc_tailroom (ofpbuf.c:290) > by 0x508D5C: ofpbuf_put_uninit (ofpbuf.c:364) > by 0x508DEF: ofpbuf_put (ofpbuf.c:387) > by 0x4CED7D: decode_bundle (ofp-actions.c:1255) > by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) > by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) > by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) > by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) > by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) > by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) > by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) > by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) > by 0x4F0D98: ofp_to_string (ofp-print.c:3453) > by 0x5486B3: do_recv (vconn.c:644) > by 0x548498: vconn_recv (vconn.c:598) > by 0x524582: rconn_recv (rconn.c:703) > by 0x45DA61: ofconn_run (connmgr.c:1370) > > Signed-off-by: Joe Stringer <joe@ovn.org> With Jarno's ack from patch #4, I pushed this to master.
On Thu, Mar 03, 2016 at 09:22:48PM +1300, Joe Stringer wrote: > Add a test which causes internal reallocation of the ofpacts buffer, > followed by a large bundle action which should cause a subsequent > reallocation while decoding slave ports. Running this test under > valgrind reveals the issue below, which is fixed in the following > commit. > > Invalid read of size 4 > at 0x4CED87: decode_bundle (ofp-actions.c:1253) > by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) > by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) > by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) > by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) > by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) > by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) > by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) > by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) > by 0x4F0D98: ofp_to_string (ofp-print.c:3453) > by 0x5486B3: do_recv (vconn.c:644) > by 0x548498: vconn_recv (vconn.c:598) > by 0x524582: rconn_recv (rconn.c:703) > by 0x45DA61: ofconn_run (connmgr.c:1370) > by 0x45B3B4: connmgr_run (connmgr.c:323) > by 0x41D1E8: ofproto_run (ofproto.c:1762) > by 0x40CEE0: bridge_run__ (bridge.c:2885) > by 0x40D093: bridge_run (bridge.c:2940) > by 0x412F7E: main (ovs-vswitchd.c:120) > Address 0x66aa460 is 1,152 bytes inside a block of size 1,184 free'd > at 0x4C2AF2E: realloc (vg_replace_malloc.c:692) > by 0x543D27: xrealloc (util.c:123) > by 0x5089EF: ofpbuf_resize__ (ofpbuf.c:243) > by 0x508B81: ofpbuf_prealloc_tailroom (ofpbuf.c:290) > by 0x508D5C: ofpbuf_put_uninit (ofpbuf.c:364) > by 0x508DEF: ofpbuf_put (ofpbuf.c:387) > by 0x4CED7D: decode_bundle (ofp-actions.c:1255) > by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) > by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) > by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) > by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) > by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) > by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) > by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) > by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) > by 0x4F0D98: ofp_to_string (ofp-print.c:3453) > by 0x5486B3: do_recv (vconn.c:644) > by 0x548498: vconn_recv (vconn.c:598) > by 0x524582: rconn_recv (rconn.c:703) > by 0x45DA61: ofconn_run (connmgr.c:1370) > > Signed-off-by: Joe Stringer <joe@ovn.org> I'd add a comment to the test explaining why it's useful. It might be a good idea to add a second command to dump the flow back out, because it's never too convincing when a test just verifies that a command exits with status 0 and no output. Usually, when a series fixes a bug and adds a test for it, we put the new test either in the same commit as the bug fix or just after the bug fix, because that maintains bisect-ability. I guess that this is less important when the bug only shows up under valgrind, but I still think that it is a sound principle.
On 5 March 2016 at 11:26, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Mar 03, 2016 at 09:22:48PM +1300, Joe Stringer wrote: >> Add a test which causes internal reallocation of the ofpacts buffer, >> followed by a large bundle action which should cause a subsequent >> reallocation while decoding slave ports. Running this test under >> valgrind reveals the issue below, which is fixed in the following >> commit. >> >> Invalid read of size 4 >> at 0x4CED87: decode_bundle (ofp-actions.c:1253) >> by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) >> by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) >> by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) >> by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) >> by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) >> by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) >> by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) >> by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) >> by 0x4F0D98: ofp_to_string (ofp-print.c:3453) >> by 0x5486B3: do_recv (vconn.c:644) >> by 0x548498: vconn_recv (vconn.c:598) >> by 0x524582: rconn_recv (rconn.c:703) >> by 0x45DA61: ofconn_run (connmgr.c:1370) >> by 0x45B3B4: connmgr_run (connmgr.c:323) >> by 0x41D1E8: ofproto_run (ofproto.c:1762) >> by 0x40CEE0: bridge_run__ (bridge.c:2885) >> by 0x40D093: bridge_run (bridge.c:2940) >> by 0x412F7E: main (ovs-vswitchd.c:120) >> Address 0x66aa460 is 1,152 bytes inside a block of size 1,184 free'd >> at 0x4C2AF2E: realloc (vg_replace_malloc.c:692) >> by 0x543D27: xrealloc (util.c:123) >> by 0x5089EF: ofpbuf_resize__ (ofpbuf.c:243) >> by 0x508B81: ofpbuf_prealloc_tailroom (ofpbuf.c:290) >> by 0x508D5C: ofpbuf_put_uninit (ofpbuf.c:364) >> by 0x508DEF: ofpbuf_put (ofpbuf.c:387) >> by 0x4CED7D: decode_bundle (ofp-actions.c:1255) >> by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) >> by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) >> by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) >> by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) >> by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) >> by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) >> by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) >> by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) >> by 0x4F0D98: ofp_to_string (ofp-print.c:3453) >> by 0x5486B3: do_recv (vconn.c:644) >> by 0x548498: vconn_recv (vconn.c:598) >> by 0x524582: rconn_recv (rconn.c:703) >> by 0x45DA61: ofconn_run (connmgr.c:1370) >> >> Signed-off-by: Joe Stringer <joe@ovn.org> > > I'd add a comment to the test explaining why it's useful. > > It might be a good idea to add a second command to dump the flow back > out, because it's never too convincing when a test just verifies that a > command exits with status 0 and no output. OK, thanks for the additional review. I'll send a follow-up patch. > Usually, when a series fixes a bug and adds a test for it, we put the > new test either in the same commit as the bug fix or just after the bug > fix, because that maintains bisect-ability. I guess that this is less > important when the bug only shows up under valgrind, but I still think > that it is a sound principle. Ack, I realised it should have been either ordered within or after the fix later on. Since the test passes even with valgrind, it seemed a little less important.
diff --git a/tests/bundle.at b/tests/bundle.at index bf62b2cae4e2..298e683b0a1a 100644 --- a/tests/bundle.at +++ b/tests/bundle.at @@ -192,3 +192,9 @@ AT_CHECK([ovs-ofctl parse-flow 'actions=bundle(symmetric_l4,60,hrw,ofport,robot: [ovs-ofctl: symmetric_l4,60,hrw,ofport,robot:1,2: missing slave delimiter, expected `slaves' got `robot' ]) AT_CLEANUP + +AT_SETUP([bundle action with many ports]) +OVS_VSWITCHD_START +AT_CHECK([ovs-ofctl add-flow br0 'actions=set_field:0x1->metadata,set_field:0x2->metadata,set_field:0x3->metadata,set_field:0x4->metadata,bundle(symmetric_l4,0,hrw,ofport,slaves:[[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40]])']) +OVS_VSWITCHD_STOP +AT_CLEANUP
Add a test which causes internal reallocation of the ofpacts buffer, followed by a large bundle action which should cause a subsequent reallocation while decoding slave ports. Running this test under valgrind reveals the issue below, which is fixed in the following commit. Invalid read of size 4 at 0x4CED87: decode_bundle (ofp-actions.c:1253) by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) by 0x4F0D98: ofp_to_string (ofp-print.c:3453) by 0x5486B3: do_recv (vconn.c:644) by 0x548498: vconn_recv (vconn.c:598) by 0x524582: rconn_recv (rconn.c:703) by 0x45DA61: ofconn_run (connmgr.c:1370) by 0x45B3B4: connmgr_run (connmgr.c:323) by 0x41D1E8: ofproto_run (ofproto.c:1762) by 0x40CEE0: bridge_run__ (bridge.c:2885) by 0x40D093: bridge_run (bridge.c:2940) by 0x412F7E: main (ovs-vswitchd.c:120) Address 0x66aa460 is 1,152 bytes inside a block of size 1,184 free'd at 0x4C2AF2E: realloc (vg_replace_malloc.c:692) by 0x543D27: xrealloc (util.c:123) by 0x5089EF: ofpbuf_resize__ (ofpbuf.c:243) by 0x508B81: ofpbuf_prealloc_tailroom (ofpbuf.c:290) by 0x508D5C: ofpbuf_put_uninit (ofpbuf.c:364) by 0x508DEF: ofpbuf_put (ofpbuf.c:387) by 0x4CED7D: decode_bundle (ofp-actions.c:1255) by 0x4CEDFC: decode_NXAST_RAW_BUNDLE (ofp-actions.c:1272) by 0x4DBDE6: ofpact_decode (ofp-actions.inc2:3765) by 0x4D6914: ofpacts_decode (ofp-actions.c:5735) by 0x4D6A3D: ofpacts_pull_openflow_actions__ (ofp-actions.c:5772) by 0x4D74F3: ofpacts_pull_openflow_instructions (ofp-actions.c:6352) by 0x4F59FA: ofputil_decode_flow_mod (ofp-util.c:1704) by 0x4EAD18: ofp_print_flow_mod (ofp-print.c:786) by 0x4F0711: ofp_to_string__ (ofp-print.c:3220) by 0x4F0D98: ofp_to_string (ofp-print.c:3453) by 0x5486B3: do_recv (vconn.c:644) by 0x548498: vconn_recv (vconn.c:598) by 0x524582: rconn_recv (rconn.c:703) by 0x45DA61: ofconn_run (connmgr.c:1370) Signed-off-by: Joe Stringer <joe@ovn.org> --- tests/bundle.at | 6 ++++++ 1 file changed, 6 insertions(+)