thread-safety: getpwuid_r()
Here is a patch to replace a getpwuid() call in the backend, for
thread-safety.
This is AFAICT the only call in the getpw*() family that needs to be
dealt with.
(There is also a getgrnam() call, but that is called very early in the
postmaster, before multiprocessing, so we can leave that as is.)
The solution here is actually quite attractive: We can replace the
getpwuid() call by the existing wrapper function pg_get_user_name(),
which internally uses getpwuid_r(). This also makes the code a bit
simpler. The same function is already used in libpq for a purpose that
mirrors the backend use, so it's also nicer to use the same function for
consistency.
Attachments:
0001-thread-safety-getpwuid_r.patchtext/plain; charset=UTF-8; name=0001-thread-safety-getpwuid_r.patchDownload+7-15
On 24/08/2024 11:42, Peter Eisentraut wrote:
Here is a patch to replace a getpwuid() call in the backend, for
thread-safety.This is AFAICT the only call in the getpw*() family that needs to be
dealt with.(There is also a getgrnam() call, but that is called very early in the
postmaster, before multiprocessing, so we can leave that as is.)The solution here is actually quite attractive: We can replace the
getpwuid() call by the existing wrapper function pg_get_user_name(),
which internally uses getpwuid_r(). This also makes the code a bit
simpler. The same function is already used in libpq for a purpose that
mirrors the backend use, so it's also nicer to use the same function for
consistency.
Makes sense.
The temporary buffers are a bit funky. pg_get_user_name() internally
uses a BUFSIZE-sized area to hold the result of getpwuid_r(). If the
pg_get_user_name() caller passes a buffer smaller than BUFSIZE, the user
id might get truncated. I don't think that's a concern on any real-world
system, and the callers do pass a large-enough buffer so truncation
can't happen. At a minimum, it would be good to add a comment to
pg_get_user_name() along the lines of "if 'buflen' is smaller than
BUFSIZE, the result might be truncated".
Come to think of it, the pg_get_user_name() function is just a thin
wrapper around getpwuid_r(). It doesn't provide a lot of value. Perhaps
we should remove pg_get_user_name() and pg_get_user_home_dir()
altogether and call getpwuid_r() directly.
But no objection to committing this as it is, either.
--
Heikki Linnakangas
Neon (https://neon.tech)
On 24.08.24 15:55, Heikki Linnakangas wrote:
Come to think of it, the pg_get_user_name() function is just a thin
wrapper around getpwuid_r(). It doesn't provide a lot of value. Perhaps
we should remove pg_get_user_name() and pg_get_user_home_dir()
altogether and call getpwuid_r() directly.
Yeah, that seems better. These functions are somewhat strangely
designed and as you described have faulty error handling. By calling
getpwuid_r() directly, we can handle the errors better and the code
becomes more transparent. (There used to be a lot more interesting
portability complications in that file, but those are long gone.)
I tried to be overly correct by using sysconf(_SC_GETPW_R_SIZE_MAX) to
get the buffer size, but that doesn't work on FreeBSD. All the OS where
I could find the source code internally use 1024 as the suggested buffer
size, so I just ended up hardcoding that. This should be no worse than
what the code is currently handling.
Attachments:
v2-0001-More-use-of-getpwuid_r-directly.patchtext/plain; charset=UTF-8; name=v2-0001-More-use-of-getpwuid_r-directly.patchDownload+72-124
On 26/08/2024 20:38, Peter Eisentraut wrote:
On 24.08.24 15:55, Heikki Linnakangas wrote:
Come to think of it, the pg_get_user_name() function is just a thin
wrapper around getpwuid_r(). It doesn't provide a lot of value.
Perhaps we should remove pg_get_user_name() and pg_get_user_home_dir()
altogether and call getpwuid_r() directly.Yeah, that seems better. These functions are somewhat strangely
designed and as you described have faulty error handling. By calling
getpwuid_r() directly, we can handle the errors better and the code
becomes more transparent. (There used to be a lot more interesting
portability complications in that file, but those are long gone.)
New patch looks good to me, thanks!
I tried to be overly correct by using sysconf(_SC_GETPW_R_SIZE_MAX) to
get the buffer size, but that doesn't work on FreeBSD. All the OS where
I could find the source code internally use 1024 as the suggested buffer
size, so I just ended up hardcoding that. This should be no worse than
what the code is currently handling.
Maybe add a brief comment on that.
--
Heikki Linnakangas
Neon (https://neon.tech)
On 26.08.24 19:54, Heikki Linnakangas wrote:
On 26/08/2024 20:38, Peter Eisentraut wrote:
On 24.08.24 15:55, Heikki Linnakangas wrote:
Come to think of it, the pg_get_user_name() function is just a thin
wrapper around getpwuid_r(). It doesn't provide a lot of value.
Perhaps we should remove pg_get_user_name() and
pg_get_user_home_dir() altogether and call getpwuid_r() directly.Yeah, that seems better. These functions are somewhat strangely
designed and as you described have faulty error handling. By calling
getpwuid_r() directly, we can handle the errors better and the code
becomes more transparent. (There used to be a lot more interesting
portability complications in that file, but those are long gone.)New patch looks good to me, thanks!
committed