switch getpwuid -> getpwuid_r #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I've always been uncomfortable with the use of getpwuid and errno. It feels gross to directly mutate a global variable even though that is what the man page for getpwuid says you are supposed to do. I think the old code was ok in terms of errno (though I was forgetting to check for NULL return values) because errno is thread-local, but I still don't like it. More worrying is that the man page says "The return value may point to a static area, and may be overwritten by subsequent calls to getpwent(3), getpwnam(), or getpwuid()." This could be an issue because user::info() is called within spawn_subshell, which itself is called right at the start of a new connection. Each connection gets its own dedicated thread, so there could potentialy be a race between two calls to user::info(). In most cases this won't be an issue since they always use the same argument (uid for the current user), but there is the potential for read/write shear on some of the `char*`s in the passwd struct. I doubt this would cause issues in practice since the kernel probably just re-uses the same pointers for a given user, but it could be a problem in theory, and would be if the kernel builds these strings at all dynamically. Using the API the plunks the memory for the returned passwd right on the stack is much safer, and does not require gross looking global accesses.
ethanpailes
force-pushed
the
fix-getpwuid
branch
from
2f6b2b3 to
c224d83
Compare
nicolasavru
approved these changes
Mar 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've always been uncomfortable with the use of getpwuid and errno. It feels gross to directly mutate a global variable even though that is what the man page for getpwuid says you are supposed to do. I think the old code was ok in terms of errno (though I was forgetting to check for NULL return values) because errno is thread-local, but I still don't like it. More worrying is that the man page says "The return value may point to a static area, and may be overwritten by subsequent calls to getpwent(3), getpwnam(), or getpwuid()." This could be an issue because user::info() is called within spawn_subshell, which itself is called right at the start of a new connection. Each connection gets its own dedicated thread, so there could potentialy be a race between two calls to user::info(). In most cases this won't be an issue since they always use the same argument (uid for the current user), but there is the potential for read/write shear on some of the
char*s in the passwd struct. I doubt this would cause issues in practice since the kernel probably just re-uses the same pointers for a given user, but it could be a problem in theory, and would be if the kernel builds these strings at all dynamically.Using the API the plunks the memory for the returned passwd right on the stack is much safer, and does not require gross looking global accesses.