Skip to content

Destroy mempool on module exit to fix memory leak #23

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.

Sign up for GitHub

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

Open
Jordymalone wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Destroy mempool on module exit to fix memory leak #23

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ static void __exit khttpd_exit(void)
send_sig(SIGTERM, http_server, 1);
kthread_stop(http_server);
close_listen_socket(listen_socket);
mempool_destroy(http_buf_pool);

@cubic-dev-ai cubic-dev-ai Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Destroying http_buf_pool on the exit path resolves the leak only if no worker thread is still using the pool at that moment. However, khttpd_exit() only stops the daemon thread (kthread_stop(http_server)); the per-connection http_server_worker kthreads spawned in http_server_daemon() are never tracked or stopped, so with an active connection a worker can still be blocked in recv holding a mempool_alloc'd buffer when mempool_destroy() frees the pool and runs http_buf_free on each element. The worker's later mempool_free(buf, http_buf_pool) then touches freed memory — converting the leak fix into a use-after-free/double-free (likely a crash) whenever unload happens with an open connection. Consider tracking all worker kthreads, stopping them (and draining/returning their buffers) before destroying the pool, so the destroy only runs once no in-flight buffers exist.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At main.c, line 187:

<comment>Destroying `http_buf_pool` on the exit path resolves the leak only if no worker thread is still using the pool at that moment. However, `khttpd_exit()` only stops the daemon thread (`kthread_stop(http_server)`); the per-connection `http_server_worker` kthreads spawned in `http_server_daemon()` are never tracked or stopped, so with an active connection a worker can still be blocked in recv holding a `mempool_alloc`'d buffer when `mempool_destroy()` frees the pool and runs `http_buf_free` on each element. The worker's later `mempool_free(buf, http_buf_pool)` then touches freed memory — converting the leak fix into a use-after-free/double-free (likely a crash) whenever unload happens with an open connection. Consider tracking all worker kthreads, stopping them (and draining/returning their buffers) before destroying the pool, so the destroy only runs once no in-flight buffers exist.</comment>

<file context>
@@ -184,6 +184,7 @@ static void __exit khttpd_exit(void)
     send_sig(SIGTERM, http_server, 1);
     kthread_stop(http_server);
     close_listen_socket(listen_socket);
+    mempool_destroy(http_buf_pool);
     pr_info("module unloaded\n");
 }
</file context>

@Jordymalone Jordymalone Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point about the pool use-after-free. That part is indeed introduced by this patch. The scenario is already fatal without it though, since workers are never tracked and kthread_run() takes no module reference. A worker blocked in recv would resume into freed module code once rmmod completes, so unloading with an open connection crashes either way. The leak fix itself is still needed for the normal unload path.

@Jordymalone Jordymalone Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jserv would you prefer to keep this patch and handle worker tracking in a follow up, fold both into this PR, or close this one?

@jserv jserv Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jserv would you prefer to keep this patch and handle worker tracking in a follow up, fold both into this PR, or close this one?

Fold here as the changes should be reasonably small.

pr_info("module unloaded\n");
}

Expand Down