Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a memory leak in ws2812_buffer_shift #3157

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

Firenox89
Copy link
Contributor

fix a memory leak in ws2812_buffer_shift by freeing the same amount of bytes we allocated before

Fixes #<GitHub-issue-number>.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

Memory was allocated with luaM_malloc(L, sizeof(ws2812_buffer_shift_prepare) + shift_len);
but freed with luaM_free(L, prepare); this caused a leak by shift_len bytes per call.

The code I used to test with:

while (true) do
    buffer:shift(10, ws2812.SHIFT_CIRCULAR)
    ws2812.write(buffer)
    print(node.egc.meminfo())
    --coroutine.yield()
end

This caused a leak of 40 bytes per call before this commit, while after the numbers are stable.

@marcelstoer marcelstoer added this to the Next release milestone Jun 12, 2020
@TerryE
Copy link
Collaborator

TerryE commented Jun 12, 2020

What this actually underlines in my mind is that we using Userdata for this type of usecase, then we can leave resource recovery to the Lua GC and this removes most paths for potential leakage.

@nwf
Copy link
Member

nwf commented Jun 12, 2020

ETA: Good find @Firenox89 and thank you for the PR!

I'm not opposed to this landing in dev now, since a fix is a fix, but I continue to hope that #2916 and my pixbuf replacement is ready for the next drop to master. https://github.com/nwf/nodemcu-firmware/blob/dev-active/app/modules/pixbuf.c already uses userdata for all its storage needs and eschews temporary allocations.

@nwf nwf merged commit 0555a4c into nodemcu:dev Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants