Make safe double remove?

[Pekka Paalanen]

On Tue, 28 Jan 2014 21:45:14 +0000
Jonathan Howard <jonathan at unbiased.name> wrote:

> More of a for argument as I see it. With unlinked state you can have
> if desired a wl_list_unlinked function (identical to empty as used on
> the head.)
> 
> Changing wl_list_insert is a code breaker as it is used to initialise
> at the moment so can't inspect the element. There remains other
> unsafe calls you should not do.
> 
> wl_list_remove changed isn't 100% guaranteed to not break anything,
> on the other hand there is no guarantee the code does not have the
> fatal double remove already. I would rather not have to debug to
> figure out remove was double called as the extremely likely fix is
> correcting that I wanted the call to be safe.

Hi,

all that seems very suspicious to me. There is something fundamentally
wrong, if you don't know how the list items or heads are supposed to be
used when using them. I think we should be making the error cases more
obvious, not hide them.

Making wl_list_remove() safe for double-remove would hide some
use-after-free errors, since object destructors usually remove the
object before freeing. I prefer we more likely crash on use-after-free
than not.

If you really want this, I could agree with adding a new function
wl_list_remove_init() or such for shaving off one line per
non-destructor remove. This function would imply "it is safe to call
remove again after this", and not "removing is safe regardless of being
removable".


Thanks,
pq

> On 28/01/14 19:36, Jason Ekstrand wrote:
> > Jonathan,
> > I have thought on multiple occations about doing this exact thing.
> > If we do, we should probably also make wl_list_insert_list safe.
> >
> > The counter-argument for all this is that you have to keep track of
> > whether or not your elements are in a list anyway to avoid
> > corruption. For instance, you still have to watch for double
> > wl_list_insert which will also break things badly.  You could say,
> > "We'll modify wl_list_insert so it's safe too" but then you
> > inserting an element that is already in a list becomes "safe" from
> > a segfault perspective and you make it easy to have program logic
> > errors that don't result in a crash, just things being subtly wrong
> > and impossible to track down.
> >
> > All said, I think I'd be in favor of making double-remove safe.
> > However, it's not a silver bullet.
> > --Jason Ekstrand
> >
> > On Jan 28, 2014 11:22 AM, "Jonathan Howard" <jonathan at unbiased.name
> > <mailto:jonathan at unbiased.name>> wrote:
> >
> >     Code is already using wl_list_init to have the construction of
> >     unlinked items in a stable state so that they can be destroyed
> > with wl_list_remove.
> >
> >     A problem is it is easy to write code that calls wl_list_remove
> >     during the structures life and miss that you need to reset with
> >     wl_list_init for when it is later destroyed.
> >
> >     Having wl_list_remove place items in this unlinked state makes
> > the destruction safe (and makes quite a few lines of code
> > redundant.)
> >
> >     --- wayland-util.c      2014-01-28 16:09:13.179052955
> >     <tel:13.179052955> +0000
> >     +++ wayland-util.c.init 2014-01-28 16:13:32.225704904 +0000
> >     @@ -52,8 +52,8 @@
> >       {
> >              elm->prev->next = elm->next;
> >              elm->next->prev = elm->prev;
> >     -       elm->next = NULL;
> >     -       elm->prev = NULL;
> >     +       elm->next = elm;
> >     +       elm->prev = elm;
> >       }
> >
> >       WL_EXPORT int