Fix skb luadata leak on netfilter hook registration failure #339
Conversation
|
Hi @lneto, I hope you’re doing well. I’ve been going through the codebase over the past few days and was looking for a small issue where I could contribute. This is my first PR, and it addresses a simple memory leak that occurred when netfilter hook registration failed after creating the skb luadata object. I’d really appreciate any feedback or suggestions. I’ve enjoyed exploring the codebase and would love to continue contributing to Lunatik in the future. |
| { | ||
| luanetfilter_t *nf = (luanetfilter_t *)private; | ||
| if (nf->skb) { | ||
| luadata_close(nf->skb); |
There was a problem hiding this comment.
good catch =), but it seems that close() isn't the proper way to release this data object.. I think we would need a detach() that would work similarly to lunatik_unregister(), that's it, cleaning up the registry (w/ nil); then, the GC will collect it accordingly.
There was a problem hiding this comment.
You’re right ,luadata_close() doesn’t remove the registry entry. I’ll add luadata_detach() (behaves like lunatik_unregisterobject()/sets registry entry to nil) so the GC can collect the object, then update the release path to use it.
| static void luanetfilter_release(void *private) | ||
| { | ||
| luanetfilter_t *nf = (luanetfilter_t *)private; | ||
| if (nf->skb) { |
There was a problem hiding this comment.
we can move the attach() call to just after setting the runtime, so we won't need this verification; just the one below.
There was a problem hiding this comment.
Okay make sense, I’ll move luadata_attach() to just after lunatik_setruntime()
| if (!nf->runtime) | ||
| return; | ||
|
|
||
| #if (LINUX_VERSION_CODE >= KERNEL_VERSION(4, 13, 0)) |
There was a problem hiding this comment.
are you able to build it to 4.x? If I recall correctly we dropped support to kernel < 5.x. Perhaps it's the case of creating another PR removing the other similar ifdefs from this module..
There was a problem hiding this comment.
README lists support for 5.x and 6.x. I can remove the 4.x #if and open a separate PR to clean up the remaining 4.x conditionals across the module unless you prefer they be included here.
lneto
left a comment
lneto
left a comment
There was a problem hiding this comment.
@sheharyaar can you please take a look at this as well?
It's not only when registration fails, right? The data object is never released if I got it correctly. |
|
@Ayushd785 could you please also take a look at other places we are using |
|
Sure, I am travelling today, I'll take a look at this tomorrow. |
yes, nf->skb wasn’t released in luanetfilter_release() so it leaks on normal cleanup too. |
absolutely, I’ll grep the repo for luadata_attach() uses and audit each release path (starting with luanetfilter.c and luaxtable.c) |
|
hii, @lneto please allow me some time i'll work on the suggestions, i am still not very much comfortable with the repo i still need a study. Once i get the core, then i'll start contributing more confidently |
of course, take your time =). thank you again! |
|
Good catch @Ayushd785! I will test this patch, once I am free and update here. |
3 participants
*Description
Fixes a memory leak that occurred when netfilter hook registration failed
after creating the skb luadata object. In this case, the object was left
registered in Lua and never released.
The cleanup path now reliably releases the skb object, ensuring correct
resource management in all failure scenarios.