Horton

Thread

Commits

  1. More use of getpwuid_r() directly

  1. thread-safety: getpwuid_r()

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-24T08:42:53Z

    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.

  2. Re: thread-safety: getpwuid_r()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-24T13:55:06Z

    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)

  3. Re: thread-safety: getpwuid_r()

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-26T17:38:59Z

    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.

  4. Re: thread-safety: getpwuid_r()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-26T17:54:13Z

    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)

  5. Re: thread-safety: getpwuid_r()

    Peter Eisentraut <peter@eisentraut.org> — 2024-09-02T07:28:21Z

    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