hpjansson / chafa

📺🗿 Terminal graphics for the 21st century.
https://hpjansson.org/chafa/
GNU Lesser General Public License v3.0
2.98k stars 64 forks source link

Possible memory leak #190

Closed jstkdng closed 9 months ago

jstkdng commented 9 months ago

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 commented 9 months ago

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.