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).
