Thread: Re: thread-safety: getpwuid_r()
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 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