KRB5: fix mem leak in authenticate_stored_users()

[alexey-tikhonov]

No description provided.

[gemini-code-assist]

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]

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]

(rebase)

[alexey-tikhonov]

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.

[sumit-bose]

/gemini review

[gemini-code-assist]

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.

[sumit-bose]

Hi,

thank you for the update, ACK.

bye,
Sumit

[alexey-tikhonov]

Note: Covscan is green.

[sssd-bot]

The pull request was accepted by @justin-stephenson with the following PR CI status:

---

🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🟢 ci / system (fedora-45) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

---

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.