Skip to content

KRB5: fix mem leak in authenticate_stored_users()#8517

Open
alexey-tikhonov wants to merge 2 commits intoSSSD:masterfrom
alexey-tikhonov:uid-table-leak
Open

KRB5: fix mem leak in authenticate_stored_users()#8517
alexey-tikhonov wants to merge 2 commits intoSSSD:masterfrom
alexey-tikhonov:uid-table-leak

Conversation

@alexey-tikhonov
Copy link
Member

No description provided.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses a memory leak in authenticate_stored_users by adding calls to hash_destroy for uid_table on all exit paths. My review focuses on ensuring the correctness of these additions. I've identified a couple of areas for improvement regarding error handling for the newly added cleanup calls.

@sumit-bose
Copy link
Contributor

Hi,

thank you for digging this issue up and fixing it. But it looks that currently get_uid_table() can return an allocated hash table even in case of an error. Which means there is a potential leak in the error checking for get_uid_table() in this function here and in init_delayed_online_authentication(). I guess it would be best to fix get_uid_table().

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

(rebase)

@alexey-tikhonov
Copy link
Member Author

But it looks that currently get_uid_table() can return an allocated hash table even in case of an error. Which means there is a potential leak in the error checking for get_uid_table() in this function here and in init_delayed_online_authentication(). I guess it would be best to fix get_uid_table().

Thanks, fixed.

@alexey-tikhonov alexey-tikhonov added coverity Trigger a coverity scan and removed Changes requested labels Mar 16, 2026
@sumit-bose
Copy link
Contributor

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses a memory leak in authenticate_stored_users() by ensuring the uid_table hash table is properly deallocated. The changes in krb5_delayed_online_authentication.c add the necessary calls to hash_destroy() on all exit paths of the function. The modification in get_uid_table() in find_uid.c enhances error handling by cleaning up the hash table upon failure, which prevents memory leaks in its callers. The changes are correct and effectively resolve the identified memory leak.

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thank you for the update, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Note: Covscan is green.

@alexey-tikhonov alexey-tikhonov removed the coverity Trigger a coverity scan label Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants