Skip to content

src/sss_client/common.c: Use getpwnam_r to avoid clobbering struct passwd#8487

Open
salahcoronya wants to merge 1 commit intoSSSD:masterfrom
salahcoronya:preserve-passwd
Open

src/sss_client/common.c: Use getpwnam_r to avoid clobbering struct passwd#8487
salahcoronya wants to merge 1 commit intoSSSD:masterfrom
salahcoronya:preserve-passwd

Conversation

@salahcoronya
Copy link
Contributor

If something else uses PAM (like openrc, see
OpenRC/openrc#984) and getpwnam, and calls something like pam_open_session, sssd's call to getpwnam in init_sssd_ids clobbers the cached value by the other program.

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 correctly replaces the non-thread-safe getpwnam with its re-entrant version, getpwnam_r, to prevent potential data corruption when multiple threads are involved. The implementation follows the standard pattern for using getpwnam_r, including dynamic buffer allocation and resizing. However, I've found a potential memory leak in the error handling of realloc and provided a suggestion to fix it.

@salahcoronya salahcoronya force-pushed the preserve-passwd branch 2 times, most recently from b66af65 to c37669a Compare March 2, 2026 05:11
@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Mar 2, 2026
@alexey-tikhonov alexey-tikhonov self-assigned this Mar 2, 2026
@alexey-tikhonov alexey-tikhonov self-requested a review March 2, 2026 08:01
@alexey-tikhonov
Copy link
Member

/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 correctly replaces the non-reentrant getpwnam with the reentrant getpwnam_r to prevent thread-safety issues. The implementation for handling ERANGE is mostly correct, but it has a potential denial-of-service vulnerability due to unbounded memory allocation. I've added a review comment with a suggested fix to address this by adding a size limit to the buffer.

@alexey-tikhonov
Copy link
Member

Hi @salahcoronya,

I think this reports a valid bug, thanks.

But there is a number of (at least) stylistic issues (re SSSD coding style) with the patch.
Would you prefer to finalize the patch yourself (and then I'll provide review comments) or do you prefer somebody from maintainers take over?

@salahcoronya
Copy link
Contributor Author

I went ahead and finalized it myself, follow the coding style document.

…sswd

If something else uses PAM (like openrc, see
OpenRC/openrc#984) and getpwnam, and calls
something like pam_open_session, sssd's call to getpwnam in
init_sssd_ids clobbers the cached value by the other program.

Signed-off-by: Christopher Byrne <salah.coronya@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-backport This should go to target branch only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants