Lv_obj_del() and kin wrongly assume that child_ll won't change while deleting

Bug (archived)
1

I am using v6.0; don’t know how pertinent this is to higher v6.x or v7.

The mechanism how this problem got visible was:

  • I have some page with a bunch of widgets,
  • widgets are put into a group
  • some widgets have .top set (using lv_obj_set_top())
  • I deleted the page (lv_obj_del())
  • lv_obj_del() starts deleting (recursively) children as they are in child_ll, always remembering the child next to itself in the ll
  • deleting a focused object invokes refocusing in lv_group.c
  • when during refocusing focus falls onto an object (let’s call it obj_x) which has .top set, this reorders the ll in question, putting this obj_x to the front of ll
  • as lv_obj_del() remembers where it left off, it continues cleaning the ll to the end, never returning to its front; thus obj_x won’t be deleted
  • lv_obj_del then deletes the parent object, thus reference to obj_x is lost, resulting in memory leak and various “funny” effects if e.g. memory is subsequently rewritten with different objects and focusing to obj_x originally resulted in starting an animation.

My fix is:

  • the entire loop deleting children in lv_obj_del() replaced by call to lv_obj_clean()

  • there is the same loop in delete_children() but I’ve removed it already as redundant as per Lv_obj_del() LV_EVENT_DELETE ordering

  • lv_obj_clean() changed to:

    void lv_obj_clean(lv_obj_t * obj) {

    lv_obj_t * child = lv_obj_get_child(obj, NULL);
    while(child) {
    lv_obj_del(child);
    child = lv_obj_get_child(obj, NULL);
    }
    }

I’m not versed in lvgl enough to be able to judge whether his has or has not any detrimental consequences; it “works for me” (I hate to say this).

JW

2

Hi,

Wow, that’s an interesting corner case.

Can you send a Pull request to better see what changed?

3

Instead of messing up another pull request (I don’t use git, and I don’t use lvgl v7), here’s diff of the proposed change:

diff --git a/../../../bak/lv_obj.c b/lv_obj.c
index 04a0bc1a..5631e114 100644
--- a/../../../bak/lv_obj.c
+++ b/lv_obj.c
@@ -485,13 +485,9 @@ void lv_obj_clean(lv_obj_t * obj)
 {
     LV_ASSERT_OBJ(obj, LV_OBJX_NAME);
     lv_obj_t * child = lv_obj_get_child(obj, NULL);
-    lv_obj_t * child_next;
     while(child) {
-        /* Read the next child before deleting the current
-         * because the next couldn't be read from a deleted (invalid) node*/
-        child_next = lv_obj_get_child(obj, child);
         lv_obj_del(child);
-        child = child_next;
+        lv_obj_get_child(obj, NULL);
     }
 }
 
@@ -3709,17 +3705,12 @@ static void obj_del_core(lv_obj_t * obj)
 
     /*Recursively delete the children*/
     lv_obj_t * i;
-    lv_obj_t * i_next;
     i = _lv_ll_get_head(&(obj->child_ll));
     while(i != NULL) {
-        /*Get the next object before delete this*/
-        i_next = _lv_ll_get_next(&(obj->child_ll), i);
-
         /*Call the recursive del to the child too*/
         obj_del_core(i);
 
-        /*Set i to the next node*/
-        i = i_next;
+        i = _lv_ll_get_head(&(obj->child_ll));
     }
 
     lv_event_mark_deleted(obj);

The point is, that whatever reordering happens in the children linked list during the delete, head of the linked list should always be available, and systematically deleting the head should be a safe way to delete the whole list.

JW

4

Got it, thanks!

In lv_obj_del() the lv_obj_get_child(obj, NULL); does nothing with the return value.
Maybe it should be child = lv_obj_get_child(obj, NULL);?

5

Indeed.

As I’ve said, I don’t use v7; I made an error when trying to “port” it from my working, sorry.

Jan

6

No problem, I’m just adding it to v7. Thank you very much for the suggestion.