On Tue, Jan 27, 2015 at 09:22:00AM +0900, Simon Horman wrote:
> 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.
[...]
> > 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.
>
> > /* 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;
> > + }
> > }
[...]
> > 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.
Are you sure? What *gms points to, that is, **gms, is freed, but *gms
should still point to the same location. list_moved() never
dereferences 'orig', only compares it against list->next. In a very
language-lawyer way, working with a pointer to freed memory may be
technically "undefined behavior", but I don't know of bad effects in
practice.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
