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

Issue 6485 - Fix double free in USN cleanup task #6486

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

mreynolds389
Copy link
Contributor

Description:

ASAN report shows double free of bind dn in the USN cleanup task data. The bind dn was passed as a reference so it should never have to be freed by the cleanup task.

Relates: #6485

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

Currently the bind_dn refers to the SLAPI_REQUESTOR_DN that was in the pblock when the task was spawn. Indeed it is freed when pblock_done and there is a double free in task_destructor. IMHO there is a risk of 'user after free' if bind_dn after it has been free by pblock_done.

If I am not missing something, I would prefer that the bind_dn is duplicated when creating the task data (usn_cleanup_add) and free during task_desctructor.

Description:

ASAN report shows double free of bind dn in the USN cleanup task data. The bind
dn was passed as a reference so it should never have to be freed by the cleanup
task.

Relates: 389ds#6485

Reviewed by: tbordaz(Thanks!)
@mreynolds389
Copy link
Contributor Author

Currently the bind_dn refers to the SLAPI_REQUESTOR_DN that was in the pblock when the task was spawn. Indeed it is freed when pblock_done and there is a double free in task_destructor. IMHO there is a risk of 'user after free' if bind_dn after it has been free by pblock_done.

If I am not missing something, I would prefer that the bind_dn is duplicated when creating the task data (usn_cleanup_add) and free during task_desctructor.

That crossed my mind, but I'm just always trying to avoid malloc/freeing when possible. Anyway you are right. There is a risk, although the window is small, it's still there. Nice catch! Fixed

@mreynolds389 mreynolds389 requested a review from tbordaz January 9, 2025 13:56
Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

LGTM

@mreynolds389 mreynolds389 merged commit 986cb5c into 389ds:main Jan 9, 2025
198 of 199 checks passed
@mreynolds389 mreynolds389 deleted the usn_asan branch January 9, 2025 18:58
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