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

Possible memory leak #190

Closed
jstkdng opened this issue Feb 3, 2024 · 1 comment
Closed

Possible memory leak #190

jstkdng opened this issue Feb 3, 2024 · 1 comment

Comments

@jstkdng
Copy link

jstkdng commented Feb 3, 2024

I was testing the apis release on 1.14, mainly chafa_canvas_print_rows_strv and chafa_canvas_print_rows and noticed that chafa_canvas_print_rows_strv uses chafa_canvas_print_rows internally.

While looking at the code I also noticed that inside chafa_canvas_print_rows_strv the GString array was not being freed by using g_free. Is that expected? or could it be a memory leak.

Thanks for your work.

@hpjansson
Copy link
Owner

I was testing the apis release on 1.14, mainly chafa_canvas_print_rows_strv and chafa_canvas_print_rows and noticed that chafa_canvas_print_rows_strv uses chafa_canvas_print_rows internally.

Yes. I think that's the most efficient way of doing it, since using GStrings in the printer allows me to preallocate space, letting the string grow if it turns out we need more space than assumed, and return the final string length so the caller can save calls to strlen(). The _strv wrapper around this just steals the GStrings' internal buffers without copying the strings themselves.

While looking at the code I also noticed that inside chafa_canvas_print_rows_strv the GString array was not being freed by using g_free. Is that expected? or could it be a memory leak.

That's a (small) leak, indeed. I'll push a fix in a minute.

Thanks for your work.

Thanks for going over the code and finding the bug! We have CI with ASan that can find these bugs in theory, but we need many more test cases to cover the entire API in the style of term-info-test.c.

hpjansson added a commit that referenced this issue Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants