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

Fix potential memory leak of tev->e_list by reordering list_add before epoll_ctl #66

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

Conversation

jkchen1095
Copy link

@jkchen1095 jkchen1095 commented Aug 10, 2024

This commit addresses a potential memory leak in tgt_event_add related to
tev->e_list. Previously, epoll_ctl(EPOLL_CTL_ADD) was called before
list_add. This sequence could lead to a race condition where the epoll event
is registered and immediately triggered, leading to EPOLL_CTL_DEL and
list_del being called before list_add, resulting in the event being removed
from epoll but never added to the list, causing a memory leak after calling
list_add (Because no one will call list_del to remove this tev->e_list anymore).

To prevent this, list_add is now called before epoll_ctl(EPOLL_CTL_ADD).
This ensures that the tev->e_list is added to the list before any
epoll events can be triggered, allowing proper cleanup by list_del in case
of an early EPOLL_CTL_DEL.

@jkchen1095 jkchen1095 force-pushed the master branch 4 times, most recently from 1ccb178 to e84dc5d Compare August 10, 2024 13:45
@jkchen1095 jkchen1095 changed the title Fix potential memory leak of event_data->e_list by reordering list_add before epoll_ctl Fix potential memory leak of tev->e_list by reordering list_add before epoll_ctl Aug 10, 2024
@jkchen1095 jkchen1095 force-pushed the master branch 5 times, most recently from a2f404a to 8a41c08 Compare August 10, 2024 14:36
…e epoll_ctl

This commit addresses a potential memory leak in tgt_event_add related to
tev->e_list. Previously, epoll_ctl(EPOLL_CTL_ADD) was called before
list_add. This sequence could lead to a race condition where the epoll event
is registered and immediately triggered, leading to EPOLL_CTL_DEL and
list_del being called before list_add, resulting in the event being removed
from epoll but never added to the list, causing a memory leak after calling
list_add (Because no one will call list_del to remove this tev->e_list anymore).

To prevent this, list_add is now called before epoll_ctl(EPOLL_CTL_ADD).
This ensures that the tev->e_list is added to the list before any
epoll events can be triggered, allowing proper cleanup by list_del in case
of an early EPOLL_CTL_DEL.

Signed-off-by: JK Chen <[email protected]>
@fujita
Copy link
Owner

fujita commented Aug 14, 2024

One thread calls tgt_event_add() and another thread calls tgt_event_del()? It's possible?

@jkchen1095
Copy link
Author

jkchen1095 commented Aug 14, 2024

Hi fujita
You are correct; in the context of a single thread, it indeed does not need to be considered. This fix is merely a formality in terms of standardization, and as long as there are no changes to a multi-threaded environment in the future, this issue will not actually arise.

@fujita
Copy link
Owner

fujita commented Aug 14, 2024

If we need to think about multi threads, there are many other races. So I don't think that fixing one place makes sense.

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

Successfully merging this pull request may close these issues.

2 participants