+1 On Jul 11, 2014, at 4:31 PM, Yann Ylavic <[email protected]> wrote:
> On Fri, Jul 11, 2014 at 10:22 PM, Yann Ylavic <[email protected]> wrote: >> There also seem to be some doubtful code in apr_skiplist_remove_all(), >> addressed by the following patch : >> >> Index: tables/apr_skiplist.c >> =================================================================== >> --- tables/apr_skiplist.c (revision 1609655) >> +++ tables/apr_skiplist.c (working copy) >> @@ -591,16 +591,17 @@ APR_DECLARE(void) apr_skiplist_remove_all(apr_skip >> m = sl->bottom; >> while (m) { >> p = m->next; >> - if (p && myfree && p->data) >> - myfree(p->data); >> + if (myfree && m->data) >> + myfree(m->data); >> while (m) { >> u = m->up; >> - apr_skiplist_free(sl, p); >> + apr_skiplist_free(sl, m); >> m = u; >> } >> m = p; >> } >> sl->top = sl->bottom = NULL; >> + sl->topend = sl->bottomend = NULL; >> sl->height = 0; >> sl->size = 0; >> } >> >> Do you agree? > > Hmm, no, the initial myfree(p->data) is on purpose, bottom is a fake > node, the real first data are on bottom->next. > > Rather this patch : > > Index: tables/apr_skiplist.c > =================================================================== > --- tables/apr_skiplist.c (revision 1609655) > +++ tables/apr_skiplist.c (working copy) > @@ -595,12 +595,13 @@ APR_DECLARE(void) apr_skiplist_remove_all(apr_skip > myfree(p->data); > while (m) { > u = m->up; > - apr_skiplist_free(sl, p); > + apr_skiplist_free(sl, m); > m = u; > } > m = p; > } > sl->top = sl->bottom = NULL; > + sl->topend = sl->bottomend = NULL; > sl->height = 0; > sl->size = 0; > } > > to avoid multiple free() on the same node. > (NULLing topend and bottomend is a safety guard since they may be used > elsewhere without top or botton being checked).
