From 04edba879de2366001bf02541ab78c9a8c8c8ca9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Aug 2023 14:35:05 +0200 Subject: [PATCH 1/4] core: use nm_strerror_native() instead of strerror() strerror() is not thread-safe. We avoid non-thread-safe API and have instead our own wrapper nm_strerror_native(). Use it. --- src/core/devices/wifi/nm-iwd-manager.c | 4 ++-- src/core/dhcp/nm-dhcp-dhcpcd.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/devices/wifi/nm-iwd-manager.c b/src/core/devices/wifi/nm-iwd-manager.c index 0d4daf215f..33efa85a61 100644 --- a/src/core/devices/wifi/nm-iwd-manager.c +++ b/src/core/devices/wifi/nm-iwd-manager.c @@ -810,7 +810,7 @@ sett_conn_changed(NMSettingsConnection *sett_conn, nm_log_dbg(LOGD_WIFI, "iwd: profile at %s not removed: %s (%i)", orig_full_path, - strerror(errno), + nm_strerror_native(errno), errno); removed = TRUE; @@ -1431,7 +1431,7 @@ try_delete_file: if (g_remove(full_path) == 0) _LOGD("IWD profile at %s removed", full_path); else if (errno != ENOENT) - _LOGD("IWD profile at %s not removed: %s (%i)", full_path, strerror(errno), errno); + _LOGD("IWD profile at %s not removed: %s (%i)", full_path, nm_strerror_native(errno), errno); } static void diff --git a/src/core/dhcp/nm-dhcp-dhcpcd.c b/src/core/dhcp/nm-dhcp-dhcpcd.c index 8d2f928ba0..7c95994ac6 100644 --- a/src/core/dhcp/nm-dhcp-dhcpcd.c +++ b/src/core/dhcp/nm-dhcp-dhcpcd.c @@ -174,7 +174,7 @@ stop(NMDhcpClient *client, gboolean release) */ if (kill(pid, sig) == -1) { errsv = errno; - _LOGE("failed to kill dhcpcd %d:%s", errsv, strerror(errsv)); + _LOGE("failed to kill dhcpcd %d:%s", errsv, nm_strerror_native(errsv)); } /* When this function exits NM expects the PID to be -1. From b53f929f4046732deb483c6746cb19876dee37b1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Aug 2023 14:42:26 +0200 Subject: [PATCH 2/4] systemd: drop strerror() define from sd adapter Systemd does not use strerror(), so this define was unused. Even if it would use it, we would better patch the upstream sources, as strerror() is not suitable in multi-threadded applications. --- .../sd-adapt-shared/nm-sd-adapt-shared.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libnm-systemd-shared/sd-adapt-shared/nm-sd-adapt-shared.h b/src/libnm-systemd-shared/sd-adapt-shared/nm-sd-adapt-shared.h index de6edd0215..83916fb2a3 100644 --- a/src/libnm-systemd-shared/sd-adapt-shared/nm-sd-adapt-shared.h +++ b/src/libnm-systemd-shared/sd-adapt-shared/nm-sd-adapt-shared.h @@ -12,11 +12,6 @@ /*****************************************************************************/ -/* strerror() is not thread-safe. Patch systemd-sources via a define. */ -#define strerror(errsv) nm_strerror_native(errsv) - -/*****************************************************************************/ - /* systemd detects whether compiler supports "-Wstringop-truncation" to disable * the warning at particular places. Since we anyway build with -Wno-pragma, * we don't do that and just let systemd call From 59251cae459a013c68e4f88794f0a9bd0deca51c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Aug 2023 14:56:42 +0200 Subject: [PATCH 3/4] std-aux: extract and add _nm_strerror_r() helper We have nm_strerror_native_r(), which is the wrapper around strerror_r() that we want to use in glib components (it also will ensure that the string is valid UTF-8). However, it's not usable from non-glib components. Move the part that abstracts strerror_r() out to libnm-std-aux as _nm_strerror_r(). The purpose is that non-glib componenent can use the thread-safe wrapper around strerror_r(). --- src/libnm-glib-aux/nm-errno.c | 23 ++++-------------- src/libnm-std-aux/nm-std-utils.c | 40 ++++++++++++++++++++++++++++++++ src/libnm-std-aux/nm-std-utils.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/libnm-glib-aux/nm-errno.c b/src/libnm-glib-aux/nm-errno.c index b76386a6fe..682bfea2a7 100644 --- a/src/libnm-glib-aux/nm-errno.c +++ b/src/libnm-glib-aux/nm-errno.c @@ -6,6 +6,7 @@ #include "libnm-glib-aux/nm-default-glib-i18n-lib.h" #include "nm-errno.h" +#include "libnm-std-aux/nm-std-utils.h" /*****************************************************************************/ @@ -100,26 +101,10 @@ nm_strerror(int nmerr) const char * nm_strerror_native_r(int errsv, char *buf, gsize buf_size) { - char *buf2; + NM_AUTO_PROTECT_ERRNO(errsv2); + const char *buf2; - nm_assert(buf); - nm_assert(buf_size > 0); - -#if (!defined(__GLIBC__) && !defined(__UCLIBC__)) || ((_POSIX_C_SOURCE >= 200112L) && !_GNU_SOURCE) - /* XSI-compliant */ - { - int errno_saved = errno; - - if (strerror_r(errsv, buf, buf_size) != 0) { - g_snprintf(buf, buf_size, "Unspecified errno %d", errsv); - errno = errno_saved; - } - buf2 = buf; - } -#else - /* GNU-specific */ - buf2 = strerror_r(errsv, buf, buf_size); -#endif + buf2 = _nm_strerror_r(errsv, buf, buf_size); /* like g_strerror(), ensure that the error message is UTF-8. */ if (!g_get_charset(NULL) && !g_utf8_validate(buf2, -1, NULL)) { diff --git a/src/libnm-std-aux/nm-std-utils.c b/src/libnm-std-aux/nm-std-utils.c index 8901378cb1..8bc109f2e2 100644 --- a/src/libnm-std-aux/nm-std-utils.c +++ b/src/libnm-std-aux/nm-std-utils.c @@ -92,3 +92,43 @@ out_huge: } return SIZE_MAX; } + +/*****************************************************************************/ + +/** + * _nm_strerror_r: + * @errsv: the errno passed to strerror_r() + * @buf: the string buffer, must be non-null + * @buf_size: the size of the buffer, must be positive. + * + * A wrapper around strerror_r(). Does little else, aside clearing up the + * confusion about the different versions of the function. + * + * errno is preserved. + * + * Returns: the error string. This is either a static strong or @buf. It + * is not guaranteed to be @buf. + */ +const char * +_nm_strerror_r(int errsv, char *buf, size_t buf_size) +{ + NM_AUTO_PROTECT_ERRNO(errsv2); + char *buf2; + + nm_assert(buf); + nm_assert(buf_size > 0); + +#if (!defined(__GLIBC__) && !defined(__UCLIBC__)) || ((_POSIX_C_SOURCE >= 200112L) && !_GNU_SOURCE) + /* XSI-compliant */ + if (strerror_r(errsv, buf, buf_size) != 0) { + snprintf(buf, buf_size, "Unspecified errno %d", errsv); + } + buf2 = buf; +#else + /* GNU-specific */ + buf2 = strerror_r(errsv, buf, buf_size); +#endif + + nm_assert(buf2); + return buf2; +} diff --git a/src/libnm-std-aux/nm-std-utils.h b/src/libnm-std-aux/nm-std-utils.h index 6aa787eb6a..015444c93d 100644 --- a/src/libnm-std-aux/nm-std-utils.h +++ b/src/libnm-std-aux/nm-std-utils.h @@ -35,4 +35,6 @@ size_t nm_utils_get_next_realloc_size(bool true_realloc, size_t requested); +const char *_nm_strerror_r(int errsv, char *buf, size_t buf_size); + #endif /* __NM_STD_UTILS_H__ */ From c42f6f0997d5399a9b212803f583a0c22983fbc5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Aug 2023 14:59:29 +0200 Subject: [PATCH 4/4] daemon-helper: use _nm_strerror_r() to avoid non-thread-safe strerror() Yes, there probably are not multiple threads here. It's a matter of principle to not use smelly functions. Also, copy the "errno" value we want to print, before calling various functions. --- src/nm-daemon-helper/nm-daemon-helper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/nm-daemon-helper/nm-daemon-helper.c b/src/nm-daemon-helper/nm-daemon-helper.c index a447d63cfe..810ea5fa94 100644 --- a/src/nm-daemon-helper/nm-daemon-helper.c +++ b/src/nm-daemon-helper/nm-daemon-helper.c @@ -94,12 +94,15 @@ cmd_resolve_address(void) NI_NAMEREQD); if (ret != 0) { if (ret == EAI_SYSTEM) { + int errsv = errno; + char buf[1024]; + fprintf(stderr, "getnameinfo() failed: %d (%s), system error: %d (%s)\n", ret, gai_strerror(ret), - errno, - strerror(errno)); + errsv, + _nm_strerror_r(errsv, buf, sizeof(buf))); } else { fprintf(stderr, "getnameinfo() failed: %d (%s)\n", ret, gai_strerror(ret)); }