Hi Ben, Hi Thomas,
On Mon, Jan 26, 2015 at 11:19:53AM -0800, Ben Pfaff wrote:
> On Mon, Jan 26, 2015 at 02:44:07PM +0100, Thomas Graf wrote:
> > On 01/26/15 at 05:30pm, Simon Horman wrote:
> > > On Mon, Jan 26, 2015 at 04:40:49PM +0900, Simon Horman wrote:
> > > > * 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.
> >
> > list_moved() not handling the list_empty() case is somewhat rude.
> > Why not just handle that special case and call list_init() in
> > list_moved() instead? I realize it's currently documented to fail in
> > list_moved() but I don't see any side effect in handling this properly
> > because any caller doing so right now would be buggy.
>
> I'd prefer to handle that case in list_moved() but I don't know a safe
> way to detect it, that is, a way that doesn't try to read from
> possibly freed memory.
Likewise.
I had an idea to detect it using something like this, but
it breaks list_is_short() and moreover changes the scheme
of empty lists.
diff --git a/lib/list.h b/lib/list.h
index 15be0f8..810eb20 100644
--- a/lib/list.h
+++ b/lib/list.h
@@ -88,7 +88,9 @@ list_init(struct ovs_list *list)
static inline void
list_poison(struct ovs_list *list)
{
- memset(list, 0xcc, sizeof *list);
+ /* next and prev must be different for list_is_empty() to work correctly */
+ memset(list, 0xcc, sizeof &list->prev);
+ memset(list, 0xdd, sizeof &list->next);
}
/* Inserts 'elem' just before 'before'. */
@@ -249,7 +251,7 @@ list_size(const struct ovs_list *list)
static inline bool
list_is_empty(const struct ovs_list *list)
{
- return list->next == list;
+ return list->prev == list->next;
}
/* Returns true if 'list' has exactly 1 element, false otherwise. */
>
> It's possible if we make list_moved() take the previous location of
> the list, e.g. something like this (compile-tested only):
This seems to solve the problem I observed. Or at least the test I included
in my patch passes.
However, I'm not sure about the correctness of the use of memory.
More on that below.
> diff --git a/lib/list.h b/lib/list.h
> index 15be0f8..a0c294e 100644
> --- a/lib/list.h
> +++ b/lib/list.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2013, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -33,7 +33,7 @@ static inline void list_splice(struct ovs_list *before,
> struct ovs_list *first,
> static inline void list_push_front(struct ovs_list *, struct ovs_list *);
> static inline void list_push_back(struct ovs_list *, struct ovs_list *);
> static inline void list_replace(struct ovs_list *, const struct ovs_list *);
> -static inline void list_moved(struct ovs_list *);
> +static inline void list_moved(struct ovs_list *, const struct ovs_list
> *orig);
> static inline void list_move(struct ovs_list *dst, struct ovs_list *src);
>
> /* List removal. */
> @@ -150,15 +150,16 @@ list_replace(struct ovs_list *element, const struct
> ovs_list *position)
> }
>
> /* Adjusts pointers around 'list' to compensate for 'list' having been moved
> - * around in memory (e.g. as a consequence of realloc()).
> - *
> - * This always works if 'list' is a member of a list, or if 'list' is the
> head
> - * of a non-empty list. It fails badly, however, if 'list' is the head of an
> - * empty list; just use list_init() in that case. */
> + * around in memory (e.g. as a consequence of realloc()), with original
> + * location 'orig'. */
> static inline void
> -list_moved(struct ovs_list *list)
> +list_moved(struct ovs_list *list, const struct ovs_list *orig)
> {
> - list->prev->next = list->next->prev = list;
> + if (list->next == orig) {
> + list_init(list);
> + } else {
> + list->prev->next = list->next->prev = list;
> + }
> }
>
> /* Initializes 'dst' with the contents of 'src', compensating for moving it
> @@ -167,12 +168,8 @@ list_moved(struct ovs_list *list)
> static inline void
> list_move(struct ovs_list *dst, struct ovs_list *src)
> {
> - if (!list_is_empty(src)) {
> - *dst = *src;
> - list_moved(dst);
> - } else {
> - list_init(dst);
> - }
> + *dst = *src;
> + list_moved(dst, src);
> }
>
> /* Removes 'elem' from its list and returns the element that followed it.
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 9acf6a4..6fc5af0 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -1413,12 +1413,14 @@ parse_ofp_group_mod_file(const char *file_name,
> uint16_t command,
> char *error;
>
> if (*n_gms >= allocated_gms) {
> + struct ofputil_group_mod *new_gms;
> size_t i;
>
> - *gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
> + new_gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
*gms might have been freed by the realloc() call indirectly made by
x2nrealloc(). But *gms is accessed below.
I realise this code was just a rough draft and this problem should
be easy enough to resolve if we decide to co with this solution.
But I wanted to take the opportunity to point it out nonetheless.
> for (i = 0; i < *n_gms; i++) {
> - list_moved(&(*gms)[i].buckets);
> + list_moved(&new_gms[i].buckets, &(*gms)[i].buckets);
> }
> + *gms = new_gms;
> }
> error = parse_ofp_group_mod_str(&(*gms)[*n_gms], command,
> ds_cstr(&s),
> &usable);
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
