Clean chart on windows simulator

Bug (archived)
,
1

Description

Hi,
I have an issue with the chart in the simulator on windows.

What MCU/Processor/Board and compiler are you using?

windows vs sdl simulator only

What do you experience?

I have a simple chart that I want to remove at a certain moment. If I use the example code for a chart this works perfect. But if I change:
#define LV_MEM_CUSTOM 1
I have an exception on line 788 of the lv_chart.c file when cleaning the series:
if(!ser->ext_buf_assigned) lv_mem_free(ser->points);

I think this is because the free function in windows is change the memory to 0xdd and therefor the _LV_LL_READ(ext->series_ll, ser) keeps on running. On other systems I don’t have this issue.

I don’t know when this bug popt up. But I believe I didn’t have it in v6.x. Right now I run the last master commit.

Code to reproduce

I just load the lv_ex_chart_1.c example.

Changed

#define LV_MEM_CUSTOM      1

added the clean function to key up just to trigger a simple clean:

        case SDLK_UP:
            /* clean the active screen as a test */
            lv_obj_clean(lv_scr_act());
            return LV_KEY_UP;

Screenshot and/or video

image
image1535Ă—529 50.2 KB

2

Hi @KoenVda,

I too have seen this issue with the Windows/Eclipse/SDL Simulator.

Having spent quite a bit of time looking at the problem, I concluded there was some memory corruption somewhere but not directly related to the chart. On that basis I decided to update all the sub-modules for the repository and the problem appears to have gone away.

So I think there may be a point in time when the sub-modules may have not worked correctly together due to many recent updates.

Hopefully if you update from the repository and update to all the latest sub-modules in your local repository you will find that things work as expected.

Kind Regards,

Pete

3

Hi @pete-pjb,

I double check my submodules but I still have the issue. Now I did some extra work on finding where the bug came up. I retested this issue with a couple of commits on master in the windows simulator

My results:
06343f18e344a2ee4eafaaf81bc02e6d42847d52 the last one for v6.x -> no issue with removing the chart
9445604456f70d839470f8dabb15f4ae511588c0 first of v7.0 -> no issue with removing the chart
a889e064371c709fe344c97a46cabd8cc5017948 v7.0.2 -> now I have the remove chart issue.
This is not a commit but I also tried v7.0.1 and here I don’t have the issue.

Then I saw a fix for the chart in the changelog for v7.0.2. related to eacc9d8ce7e42b58368515d27669e83a4347c011
and indeed this is triggering the error for me.

possible solution?

what if you just remove line 657 -> lv_mem_free(ser);
don’t know if this is an mem leak because right after _lv_ll_clear(&ext->series_ll); is called

4

Hi @KoenVda,

I owe you an apology it seems, upon further investigation my eclipse workspace had gotten itself into a pickle and somehow reverted the LVGL repository back to V7.0.1 so what I said was incorrect yesterday!

As far as I can see the lv_mem_free(ser) should definitely be there and as you pointed out if you use the LVGL memory management it works just fine. So it’s an issue when the libc; malloc() and free() functions are being used instead.

I will take another look at this, it might be good if @kisvegabor could have a look if he has time also…

Kind Regards,

Pete

5

This behavior is indeed suspicious. I believe what’s happening here is a typical use-after-free bug: the element is being freed and then used to find the next entry in the list. Take a look at the _LV_LL_READ implementation; you’ll see what I mean.

I ran into a similar bug in a different project using linked lists not too long ago. The solution was to find the next entry in the list first and then free the current entry. Not sure if that’s possible here.

6

Hi @embeddedt,

Here is a work around which appears to work correctly.

If you are happy with it would you like me to add it to the master branch?

static lv_res_t lv_chart_signal(lv_obj_t * chart, lv_signal_t sign, void * param)
{
    /* Include the ancient signal function */
    lv_res_t res;
    if(sign == LV_SIGNAL_GET_STYLE) {
        lv_get_style_info_t * info = param;
        info->result = lv_chart_get_style(chart, info->part);
        if(info->result != NULL) return LV_RES_OK;
        else return ancestor_signal(chart, sign, param);
    }

    res = ancestor_signal(chart, sign, param);
    if(res != LV_RES_OK) return res;
    if(sign == LV_SIGNAL_GET_TYPE) return lv_obj_handle_get_type_signal(param, LV_OBJX_NAME);

    lv_chart_ext_t * ext = lv_obj_get_ext_attr(chart);

    if(sign == LV_SIGNAL_CLEANUP) {
        lv_chart_series_t * ser;
        lv_ll_node_t * ll_tmp_head;
        while( ext->series_ll.head != NULL ) {
        	ser = (lv_chart_series_t *)ext->series_ll.head;
            if(ext->series_ll.head != ext->series_ll.tail) {
            	ll_tmp_head = ext->series_ll.tail;
            } else ll_tmp_head = NULL;

            if(!ser->ext_buf_assigned) lv_mem_free(ser->points);
            lv_mem_free(ser);

            ext->series_ll.head= ll_tmp_head;
        }
        _lv_ll_clear(&ext->series_ll);

        lv_obj_clean_style_list(chart, LV_CHART_PART_SERIES);
        lv_obj_clean_style_list(chart, LV_CHART_PART_SERIES_BG);
    }

    return res;
}

Also is there likely to be any other areas of the library where this could happen? Do we need to check?

Kind Regards,

Pete

7

Since making the previous, post I set up a quick test in the simulator using the LVGL memory management just to make sure there are no leaks now. It’s just basically an lv_task which creates a chart if none exists and deletes a chart if it does exist which executes every 5 seconds and here is the memory monitor console output…

used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968
used:   6096 ( 10 %), frag:   0 %, biggest free:  59440
used:   5520 (  9 %), frag:   1 %, biggest free:  59968

Kind Regards,

Pete

8

This issue came up many places in the library. Most of them was solved by me in the early day of lvgl but seemingly this one was uncovered.

@pete-pjb’s approach solution looks good altroght this is easier to read for me:

 while( ext->series_ll.head != NULL ) {
   ser =_lv_ll_get_head(&ext->series_ll);
   
   if(!ser->ext_buf_assigned) lv_mem_free(ser->points);
  
  _lv_ll_remove(&ext->series_ll, ser);
  lv_mem_free(ser);
}

What do you think?

2 Likes
9

Looks good to me. @pete-pjb Do you want to commit it?

10

Hi @embeddedt,

I will do it shortly, no problem :+1:

Kind Regards,

Pete

11

Hi @KoenVda, @embeddedt, @kisvegabor,

I have pushed a hot fix here, @KoenVda would you be able to test and let me know if all is well please.

Thank you,

Kind Regards,

Pete

1 Like
12

Hi @pete-pjb @embeddedt @kisvegabor ,

Thanks for the update it works perfect!

Koen

13

No worries Koen, thank you for letting us know, and bringing the bug to our attention.

Kind Regards,

Pete

14

Thanks Pete! I marked your commit as solution.

1 Like