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 when creating a region #8

Open
psychon opened this issue Dec 8, 2019 · 4 comments
Open

Possible memory leak when creating a region #8

psychon opened this issue Dec 8, 2019 · 4 comments

Comments

@psychon
Copy link
Member

psychon commented Dec 8, 2019

I just noticed a possible memory leak:

oocairo/obj_region.c

Lines 40 to 50 in 2aba6f9

static int
region_create_rectangles (lua_State *L) {
size_t max_entries = lua_objlen (L, 1);
cairo_rectangle_int_t *rects = calloc(max_entries, sizeof(*rects));
size_t i = 0;
/* Now iterate over the table */
lua_pushnil(L);
while (lua_next(L, 1) != 0) {
if (i >= max_entries)
return luaL_error(L, "Cairo.region_create_rectangles(): Internal error, table larger than table?!");

The calloc result is leaked if an error happens (either via luaL_error or via lua_next doing a longjmp).

@osch
Copy link
Contributor

osch commented Dec 10, 2019

I'm working on this... (see osch@0d58a59). Perhaps there are some more dark corners around here...

@psychon
Copy link
Member Author

psychon commented Dec 10, 2019

I was thinking... how about adding an internal kind of userdata that just does free(*value) in its __gc metamethod? This would mean (slightly) more memory use, but would guarantee "leak-freeness" relatively automatically. One would then just set the internal value to NULL to "disarm" such a guard in case one wants to continue using the memory.

That would be a more local code change, I think. If you want, I can try working on this.

@osch
Copy link
Contributor

osch commented Dec 10, 2019

how about adding an internal kind of userdata that just does free(*value) in its __gc metamethod?

This would generally be a useful tool, perhaps there are more places where this can be useful.

This would mean (slightly) more memory use

One could additional free the memory manually, so it would only be freed in the the garbage collector in error case.

However I would also modify the current code as I suggested because I think there is more wrong here than the possible leak. E.g I would not iterate over all key values but only over the integer keys, see my code example.

@osch
Copy link
Contributor

osch commented Dec 11, 2019

This would mean (slightly) more memory use

One could additional free the memory manually, so it would only be freed in the the garbage collector in error case.

I thought again over this: we could avoid additional memory overhead by creating a userdata with a free in the the __gc metamethod and calling free also at the end of the function. This comes at the cost of two allocations or we can have only one allocation (userdata big enough) which is than freed when the garbage collector runs.

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