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
From 090c800afd6271885d345f72bbd1d3b535dd6886 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 24 Aug 2024 10:35:56 +0200
Subject: [PATCH] thread-safety: getpwuid_r()
There was one getpwuid() call in the backend. We can replace that one
by the existing wrapper function pg_get_user_name(), which internally
uses getpwuid_r() for thread-safety. And this also makes the code a
bit simpler. The same function is already used in libpq for a purpose
that mirrors the backend use.
---
src/backend/libpq/auth.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 2b607c52704..85dcf9e9fc9 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1857,7 +1857,7 @@ auth_peer(hbaPort *port)
uid_t uid;
gid_t gid;
#ifndef WIN32
- struct passwd *pw;
+ char pwdbuf[BUFSIZ];
int ret;
#endif
@@ -1876,25 +1876,18 @@ auth_peer(hbaPort *port)
}
#ifndef WIN32
- errno = 0; /* clear errno before call */
- pw = getpwuid(uid);
- if (!pw)
+ if (!pg_get_user_name(uid, pwdbuf, sizeof pwdbuf))
{
- int save_errno = errno;
-
- ereport(LOG,
- (errmsg("could not look up local user ID %ld: %s",
- (long) uid,
- save_errno ? strerror(save_errno) : _("user does not exist"))));
+ ereport(LOG, errmsg_internal("%s", pwdbuf));
return STATUS_ERROR;
}
/*
- * Make a copy of static getpw*() result area; this is our authenticated
- * identity. Set it before calling check_usermap, because authentication
- * has already succeeded and we want the log file to reflect that.
+ * Make a copy of result; this is our authenticated identity. Set it
+ * before calling check_usermap, because authentication has already
+ * succeeded and we want the log file to reflect that.
*/
- set_authn_id(port, pw->pw_name);
+ set_authn_id(port, pwdbuf);
ret = check_usermap(port->hba->usermap, port->user_name,
MyClientConnectionInfo.authn_id, false);
--
2.46.0
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
From 36a1e9a3860edfa8fedbeb8abfd2b0f0db2a0676 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 26 Aug 2024 19:28:08 +0200
Subject: [PATCH v2] More use of getpwuid_r() directly
Remove src/port/user.c, call getpwuid_r() directly. This reduces some
complexity and allows better control of the error behavior.
Also convert src/backend/libpq/auth.c to use getpwuid_r() for
thread-safety.
---
src/backend/libpq/auth.c | 21 +++++---
src/bin/psql/nls.mk | 3 +-
src/include/port.h | 6 ---
src/interfaces/libpq/fe-auth.c | 23 ++++++--
src/interfaces/libpq/fe-connect.c | 23 ++++++--
src/interfaces/libpq/nls.mk | 3 +-
src/port/Makefile | 3 +-
src/port/meson.build | 1 -
src/port/path.c | 23 ++++++--
src/port/user.c | 89 -------------------------------
10 files changed, 72 insertions(+), 123 deletions(-)
delete mode 100644 src/port/user.c
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 2b607c52704..47e8c916060 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1857,7 +1857,10 @@ auth_peer(hbaPort *port)
uid_t uid;
gid_t gid;
#ifndef WIN32
+ struct passwd pwbuf;
struct passwd *pw;
+ char buf[1024];
+ int rc;
int ret;
#endif
@@ -1876,16 +1879,18 @@ auth_peer(hbaPort *port)
}
#ifndef WIN32
- errno = 0; /* clear errno before call */
- pw = getpwuid(uid);
- if (!pw)
+ rc = getpwuid_r(uid, &pwbuf, buf, sizeof buf, &pw);
+ if (rc != 0)
+ {
+ errno = rc;
+ ereport(LOG,
+ errmsg("could not look up local user ID %ld: %m", (long) uid));
+ return STATUS_ERROR;
+ }
+ else if (!pw)
{
- int save_errno = errno;
-
ereport(LOG,
- (errmsg("could not look up local user ID %ld: %s",
- (long) uid,
- save_errno ? strerror(save_errno) : _("user does not exist"))));
+ errmsg("local user with ID %ld does not exist", (long) uid));
return STATUS_ERROR;
}
diff --git a/src/bin/psql/nls.mk b/src/bin/psql/nls.mk
index 7fd8fedead4..ef80163f9a4 100644
--- a/src/bin/psql/nls.mk
+++ b/src/bin/psql/nls.mk
@@ -23,8 +23,7 @@ GETTEXT_FILES = $(FRONTEND_COMMON_GETTEXT_FILES) \
../../common/exec.c \
../../common/fe_memutils.c \
../../common/username.c \
- ../../common/wait_error.c \
- ../../port/user.c
+ ../../common/wait_error.c
GETTEXT_TRIGGERS = $(FRONTEND_COMMON_GETTEXT_TRIGGERS) \
HELP0 HELPN N_ simple_prompt simple_prompt_extended
GETTEXT_FLAGS = $(FRONTEND_COMMON_GETTEXT_FLAGS) \
diff --git a/src/include/port.h b/src/include/port.h
index c7400052675..ba9ab0d34f4 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -436,12 +436,6 @@ extern size_t strnlen(const char *str, size_t maxlen);
extern char *strsep(char **stringp, const char *delim);
#endif
-/* port/user.c */
-#ifndef WIN32
-extern bool pg_get_user_name(uid_t user_id, char *buffer, size_t buflen);
-extern bool pg_get_user_home_dir(uid_t user_id, char *buffer, size_t buflen);
-#endif
-
/*
* Callers should use the qsort() macro defined below instead of calling
* pg_qsort() directly.
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 5c8f404463f..4904d38ce1c 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -28,6 +28,7 @@
#include <unistd.h>
#include <fcntl.h>
#include <limits.h>
+#include <pwd.h>
#include <sys/param.h> /* for MAXHOSTNAMELEN on most */
#include <sys/socket.h>
#ifdef HAVE_SYS_UCRED_H
@@ -1203,7 +1204,10 @@ pg_fe_getusername(uid_t user_id, PQExpBuffer errorMessage)
char username[256 + 1];
DWORD namesize = sizeof(username);
#else
- char pwdbuf[BUFSIZ];
+ struct passwd pwbuf;
+ struct passwd *pw;
+ char buf[1024];
+ int rc;
#endif
#ifdef WIN32
@@ -1214,10 +1218,19 @@ pg_fe_getusername(uid_t user_id, PQExpBuffer errorMessage)
"user name lookup failure: error code %lu",
GetLastError());
#else
- if (pg_get_user_name(user_id, pwdbuf, sizeof(pwdbuf)))
- name = pwdbuf;
- else if (errorMessage)
- appendPQExpBuffer(errorMessage, "%s\n", pwdbuf);
+ rc = getpwuid_r(user_id, &pwbuf, buf, sizeof buf, &pw);
+ if (rc != 0)
+ {
+ errno = rc;
+ if (errorMessage)
+ libpq_append_error(errorMessage, "could not look up local user ID %ld: %m", (long) user_id);
+ }
+ else if (!pw)
+ {
+ if (errorMessage)
+ libpq_append_error(errorMessage, "local user with ID %ld does not exist", (long) user_id);
+ }
+ name = pw->pw_name;
#endif
if (name)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index ab308a0580f..4cd7281b6ed 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -50,6 +50,7 @@
#include <netdb.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
+#include <pwd.h>
#endif
#ifdef WIN32
@@ -7702,10 +7703,24 @@ pqGetHomeDirectory(char *buf, int bufsize)
const char *home;
home = getenv("HOME");
- if (home == NULL || home[0] == '\0')
- return pg_get_user_home_dir(geteuid(), buf, bufsize);
- strlcpy(buf, home, bufsize);
- return true;
+ if (home && home[0])
+ {
+ strlcpy(buf, home, bufsize);
+ return true;
+ }
+ else
+ {
+ struct passwd pwbuf;
+ struct passwd *pw;
+ char tmpbuf[1024];
+ int rc;
+
+ rc = getpwuid_r(geteuid(), &pwbuf, tmpbuf, sizeof tmpbuf, &pw);
+ if (rc != 0 || !pw)
+ return false;
+ strlcpy(buf, pw->pw_dir, bufsize);
+ return true;
+ }
#else
char tmppath[MAX_PATH];
diff --git a/src/interfaces/libpq/nls.mk b/src/interfaces/libpq/nls.mk
index 4788088eb93..ae761265852 100644
--- a/src/interfaces/libpq/nls.mk
+++ b/src/interfaces/libpq/nls.mk
@@ -13,8 +13,7 @@ GETTEXT_FILES = fe-auth.c \
fe-secure-common.c \
fe-secure-gssapi.c \
fe-secure-openssl.c \
- win32.c \
- ../../port/user.c
+ win32.c
GETTEXT_TRIGGERS = libpq_append_conn_error:2 \
libpq_append_error:2 \
libpq_gettext \
diff --git a/src/port/Makefile b/src/port/Makefile
index db7c02117b0..9324ec2d9fc 100644
--- a/src/port/Makefile
+++ b/src/port/Makefile
@@ -57,8 +57,7 @@ OBJS = \
quotes.o \
snprintf.o \
strerror.o \
- tar.o \
- user.o
+ tar.o
# libpgport.a, libpgport_shlib.a, and libpgport_srv.a contain the same files
# foo.o, foo_shlib.o, and foo_srv.o are all built from foo.c
diff --git a/src/port/meson.build b/src/port/meson.build
index ff54b7b53e9..1150966ab71 100644
--- a/src/port/meson.build
+++ b/src/port/meson.build
@@ -20,7 +20,6 @@ pgport_sources = [
'snprintf.c',
'strerror.c',
'tar.c',
- 'user.c',
]
if host_system == 'windows'
diff --git a/src/port/path.c b/src/port/path.c
index 330b3f90332..de4df6cd78b 100644
--- a/src/port/path.c
+++ b/src/port/path.c
@@ -32,6 +32,7 @@
#define near
#include <shlobj.h>
#else
+#include <pwd.h>
#include <unistd.h>
#endif
@@ -934,10 +935,24 @@ get_home_path(char *ret_path)
const char *home;
home = getenv("HOME");
- if (home == NULL || home[0] == '\0')
- return pg_get_user_home_dir(geteuid(), ret_path, MAXPGPATH);
- strlcpy(ret_path, home, MAXPGPATH);
- return true;
+ if (home && home[0])
+ {
+ strlcpy(ret_path, home, MAXPGPATH);
+ return true;
+ }
+ else
+ {
+ struct passwd pwbuf;
+ struct passwd *pw;
+ char buf[1024];
+ int rc;
+
+ rc = getpwuid_r(geteuid(), &pwbuf, buf, sizeof buf, &pw);
+ if (rc != 0 || !pw)
+ return false;
+ strlcpy(ret_path, pw->pw_dir, MAXPGPATH);
+ return true;
+ }
#else
char *tmppath;
diff --git a/src/port/user.c b/src/port/user.c
deleted file mode 100644
index 7444aeb64b2..00000000000
--- a/src/port/user.c
+++ /dev/null
@@ -1,89 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * user.c
- *
- * Wrapper functions for user and home directory lookup.
- *
- * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
- *
- * src/port/user.c
- *
- *-------------------------------------------------------------------------
- */
-
-#include "c.h"
-
-#include <pwd.h>
-
-#ifndef WIN32
-
-/*
- * pg_get_user_name - get the name of the user with the given ID
- *
- * On success, the user name is returned into the buffer (of size buflen),
- * and "true" is returned. On failure, a localized error message is
- * returned into the buffer, and "false" is returned.
- */
-bool
-pg_get_user_name(uid_t user_id, char *buffer, size_t buflen)
-{
- char pwdbuf[BUFSIZ];
- struct passwd pwdstr;
- struct passwd *pw = NULL;
- int pwerr;
-
- pwerr = getpwuid_r(user_id, &pwdstr, pwdbuf, sizeof(pwdbuf), &pw);
- if (pw != NULL)
- {
- strlcpy(buffer, pw->pw_name, buflen);
- return true;
- }
- if (pwerr != 0)
- snprintf(buffer, buflen,
- _("could not look up local user ID %d: %s"),
- (int) user_id,
- strerror_r(pwerr, pwdbuf, sizeof(pwdbuf)));
- else
- snprintf(buffer, buflen,
- _("local user with ID %d does not exist"),
- (int) user_id);
- return false;
-}
-
-/*
- * pg_get_user_home_dir - get the home directory of the user with the given ID
- *
- * On success, the directory path is returned into the buffer (of size buflen),
- * and "true" is returned. On failure, a localized error message is
- * returned into the buffer, and "false" is returned.
- *
- * Note that this does not incorporate the common behavior of checking
- * $HOME first, since it's independent of which user_id is queried.
- */
-bool
-pg_get_user_home_dir(uid_t user_id, char *buffer, size_t buflen)
-{
- char pwdbuf[BUFSIZ];
- struct passwd pwdstr;
- struct passwd *pw = NULL;
- int pwerr;
-
- pwerr = getpwuid_r(user_id, &pwdstr, pwdbuf, sizeof(pwdbuf), &pw);
- if (pw != NULL)
- {
- strlcpy(buffer, pw->pw_dir, buflen);
- return true;
- }
- if (pwerr != 0)
- snprintf(buffer, buflen,
- _("could not look up local user ID %d: %s"),
- (int) user_id,
- strerror_r(pwerr, pwdbuf, sizeof(pwdbuf)));
- else
- snprintf(buffer, buflen,
- _("local user with ID %d does not exist"),
- (int) user_id);
- return false;
-}
-
-#endif /* !WIN32 */
base-commit: 8daa62a10c911c851f7e9ec5ef7b90cfd4b73212
--
2.46.0
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