Thanks for catching this, Simon.
Pushed to master,
Jarno
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
On Apr 7, 2014, at 1:43 AM, Simon Horman <ho...@verge.net.au> wrote:
> A call to ofpbuf_put() may cause the data of the passed to be resized and
> its base address to change. Thus the address returned by ofpbuf_put()
> should be used as the base address rather than relying on the base address
> prior to ofpbuf_put().
>
> This avoids the following assertion in ofpact_update_len() from failing
> when ofpbuf_put() causes a resize in parse_note(), parse_noargs_dec_ttl(),
> parse_dec_ttl().
>
> ovs_assert(ofpact == ofpacts->frame);
>
> This restores pointer updates removed by and resolves a regression
> introduced by cf3b7538666cd1ef ("ofpbuf: Abstract 'l2' pointer and document
> usage conventions.").
>
> Test cases have also been added to exercise this buffer resize in
> parse_note(), parse_noargs_dec_ttl(), parse_dec_ttl().
>
> Cc: Jarno Rajahalme <jrajaha...@nicira.com>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
> ---
> lib/ofp-parse.c | 3 +++
> tests/ofproto-dpif.at | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 92fb40f..2ada88d 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -291,6 +291,7 @@ parse_note(const char *arg, struct ofpbuf *ofpacts)
> }
> ofpbuf_put(ofpacts, &byte, 1);
>
> + note = ofpacts->frame;
> note->length++;
>
> arg += 2;
> @@ -399,6 +400,7 @@ parse_noargs_dec_ttl(struct ofpbuf *b)
>
> ids = ofpact_put_DEC_TTL(b);
> ofpbuf_put(b, &id, sizeof id);
> + ids = b->frame;
> ids->n_controllers++;
> ofpact_update_len(b, &ids->ofpact);
> }
> @@ -424,6 +426,7 @@ parse_dec_ttl(struct ofpbuf *b, char *arg)
> uint16_t id = atoi(cntr);
>
> ofpbuf_put(b, &id, sizeof id);
> + ids = b->frame;
> ids->n_controllers++;
> }
> if (!ids->n_controllers) {
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index f2ddda9..441cdf2 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -336,6 +336,56 @@
> ip,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +dnl A dec_ttl action at offset 32 in ofpacts will cause the ofpacts
> +dnl buffer to be resized just before pushing the id of the dec_ttl action.
> +dnl Thus the implementation must account for this by using the
> +dnl reallocated buffer rather than the original buffer.
> +dnl
> +dnl A number of similar rules are added to try and exercise
> +dnl xrealloc sufficiently that it returns a different base pointer
> +AT_SETUP([ofproto-dpif - dec_ttl without arguments at offset 32 in ofpacts])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], [1])
> +(for i in `seq 0 255`; do
> + printf "dl_src=10:11:11:11:11:%02x
> actions=output:1,output:1,output:1,dec_ttl,controller\n" $i
> + done) > flows.txt
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +dnl A dec_ttl action at offset 32 in ofpacts will cause the ofpacts
> +dnl buffer to be resized just before pushing the id of the dec_ttl action.
> +dnl Thus the implementation must account for this by using the
> +dnl reallocated buffer rather than the original buffer.
> +dnl
> +dnl A number of similar rules are added to try and exercise
> +dnl xrealloc sufficiently that it returns a different base pointer
> +AT_SETUP([ofproto-dpif - dec_ttl with arguments at offset 32 in ofpacts])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], [1])
> +(for i in `seq 0 255`; do
> + printf "dl_src=10:11:11:11:11:%02x
> actions=output:1,output:1,output:1,dec_ttl(1),controller\n" $i
> + done) > flows.txt
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +dnl A note action at offset 24 in ofpacts will cause the ofpacts
> +dnl buffer to be resized just before pushing the id of the dec_ttl action.
> +dnl Thus the implementation must account for this by using the
> +dnl reallocated buffer rather than the original buffer.
> +dnl
> +dnl A number of similar rules are added to try and exercise
> +dnl xrealloc sufficiently that it returns a different base pointer
> +AT_SETUP([ofproto-dpif - note at offset 24 in ofpacts])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], [1])
> +(for i in `seq 0 255`; do
> + printf "dl_src=10:11:11:11:11:%02x
> actions=output:1,output:1,note:ff,controller\n" $i
> + done) > flows.txt
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>
> AT_SETUP([ofproto-dpif - output, OFPP_NONE ingress port])
> OVS_VSWITCHD_START
> --
> 1.8.5.2
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
