On Aug 26, 2014, at 3:01 AM, Thomas Graf <tg...@noironetworks.com> wrote:
> The branch is unused as size < sizeof dst->inline_values must
> always be true for inlined values. Hitting the branch would lead
> to corruption as inline_values is accessed out of bounds.
>
Miniflows can also be dynamically allocated to have more inline space than the
default amount. classifier.c makes use of that, but it never calls
miniflow_move (even indirectly) so you are not getting an assert fail. The
motivation for right-sized miniflows in the classifier is to reduce levels of
indirection while not wasting memory.
I should (have) extend(ed) the comment above struct miniflow definition in
lib/flow.h to describe the above.
size is computed from the actual source miniflow instance, not from the generic
miniflow struct. If size really is larger than the default inline space, then
obviously no out of bounds access would occur.
I’m OK changing this function to not work for dynamically allocated miniflows,
but that should be reflected in the comment above the function, which currently
reads:
/* Initializes 'dst' with the data in 'src', destroying 'src'.
* The caller must eventually free 'dst' with miniflow_destroy().
* 'dst' must be regularly sized miniflow, but 'src' can have
* larger than default inline values. */
However, it might be better to revert this patch to keep the
larger-than-default miniflows not stand out too much.
Regards,
Jarno
> Remove branch and add assertion.
>
> Cc: Jarno Rajahalme <jrajaha...@nicira.com>
> Signed-off-by: Thomas Graf <tg...@noironetworks.com>
> ---
> v2: fixed assertion
>
> lib/flow.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index 29b331e..bef2f27 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1726,12 +1726,9 @@ miniflow_move(struct miniflow *dst, struct miniflow
> *src)
> dst->values_inline = true;
> memcpy(dst->inline_values, miniflow_get_values(src), size);
> miniflow_destroy(src);
> - } else if (src->values_inline) {
> - dst->values_inline = false;
> - COVERAGE_INC(miniflow_malloc);
> - dst->offline_values = xmalloc(size);
> - memcpy(dst->offline_values, src->inline_values, size);
> } else {
> + ovs_assert(!src->values_inline);
> +
> dst->values_inline = false;
> dst->offline_values = src->offline_values;
> }
> --
> 1.9.3
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
