On Mon, Jan 26, 2015 at 04:40:49PM +0900, Simon Horman wrote:
> list_moved() appears to only work reliably if its argument is a non-empty
> list. This is because list_moved dereferences the next and prev fields
> of the list, which for an empty list are the address of the list itself,
> and list_moved() is intended to be called when the address of the list has
> changed.
>
> This patch updates the only user of list_moved() that I was able to find
> my temporarily setting the next field to NULL if it is empty and then
> re-initialising the list rather than calling list_moved() after
> reallocation has occurred.
>
> An test update is also provided to exercise this change. Without
> the code change the updated test cases a segmentation when the
> buckets parameter of ofputil_put_ofp11_bucket() is dereferenced.
>
> Signed-off-by: Simon Horman <simon.hor...@netronome.com>
>
> ---
>
> * Although somewhat cure the approach of setting the next field to NULL
s/cure/crude/
> seems far less dangerous than trying to update the list infrastructure
> to handle this case.
>
> I also toyed with using a temporary bitmap to track empty buckets.
> That approach worked but seemed to be more overhead and no cleaner
> than the one taken by this patch.
> ---
> lib/ofp-parse.c | 11 ++++++++++-
> tests/ofproto.at | 13 +++++++++++--
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 9acf6a4..9a5df3b 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -1415,9 +1415,18 @@ parse_ofp_group_mod_file(const char *file_name,
> uint16_t command,
> if (*n_gms >= allocated_gms) {
> size_t i;
>
> + for (i = 0; i < *n_gms; i++) {
> + if (list_is_empty(&(*gms)[i].buckets)) {
> + (*gms)[i].buckets.next = NULL;
> + }
> + }
> *gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
> for (i = 0; i < *n_gms; i++) {
> - list_moved(&(*gms)[i].buckets);
> + if ((*gms)[i].buckets.next) {
> + list_moved(&(*gms)[i].buckets);
> + } else {
> + list_init(&(*gms)[i].buckets);
> + }
> }
> }
> error = parse_ofp_group_mod_str(&(*gms)[*n_gms], command,
> ds_cstr(&s),
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 2a2111f..39978e4 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -286,16 +286,25 @@ dnl Actions definition listed in both supported formats
> (w/ actions=)
> AT_SETUP([ofproto - del group (OpenFlow 1.1)])
> OVS_VSWITCHD_START
> AT_DATA([groups.txt], [dnl
> +group_id=1231,type=all
> +group_id=1232,type=all
> +group_id=1233,type=all
> group_id=1234,type=all,bucket=output:10
> group_id=1235,type=all,bucket=actions=output:10
> ])
> AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-groups br0 groups.txt])
> AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0 ], [0], [stdout])
> -AT_CHECK([STRIP_XIDS stdout], [0], [dnl
> -OFPST_GROUP_DESC reply (OF1.1):
> +AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl
> + group_id=1231,type=all
> + group_id=1232,type=all
> + group_id=1233,type=all
> group_id=1234,type=all,bucket=actions=output:10
> group_id=1235,type=all,bucket=actions=output:10
> +OFPST_GROUP_DESC reply (OF1.1):
> ])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1231])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1232])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1233])
> AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1234])
> AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0], [0], [stdout])
> AT_CHECK([STRIP_XIDS stdout], [0], [dnl
> --
> 2.1.4
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
