From 0d5b04ad9cbb783cf0d71d1587aecb8470c21d31 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Oct 2022 14:42:25 +0200 Subject: [PATCH 1/8] glib-aux/tests: reinclude with NDEBUG undefined While we usually don't do that, we also want to build with NDEBUG. But in that case, we don't want that the assertions from our unit tests are disabled. Solve that by undefining NDEBUG and re-including . --- src/libnm-glib-aux/nm-test-utils.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index e884534727..6b4ed18916 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -85,6 +85,9 @@ #undef g_assertion_message_expr #endif +#undef NDEBUG +#include + #include #include #include From 197963fba31cc8e5ce9c338ca0a69cee3d997adb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Oct 2022 11:18:27 +0200 Subject: [PATCH 2/8] std-aux: add _nm_noreturn macro --- src/libnm-std-aux/nm-std-aux.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index 96e379926f..aad8931207 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -20,6 +20,7 @@ #define _nm_used __attribute__((__used__)) #define _nm_pure __attribute__((__pure__)) #define _nm_const __attribute__((__const__)) +#define _nm_noreturn __attribute__((__noreturn__)) #define _nm_warn_unused_result __attribute__((__warn_unused_result__)) #define _nm_printf(a, b) __attribute__((__format__(__printf__, a, b))) #define _nm_align(s) __attribute__((__aligned__(s))) From 8e3299498dd1f4c131b12f9b4517bfb3f6bbbba1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Oct 2022 08:49:51 +0200 Subject: [PATCH 3/8] std-aux,glib-aux: rework nm_assert() implementations 1) ensure the compiler always sees the condition (even if it's unreachable). That is important, to avoid warnings about unused variables and to ensure the condition compiles. Previously, if NM_MORE_ASSERTS was enabled and NDEBUG (or G_DISABLE_ASSERT) was defined, the condition was not seen by the compiler. 2) to achieve point 1, we evaluate the expression now always in nm_assert() macro directly. This also has the benefit that we control exactly what is done there, and the implementation is guaranteed to not use any code that is not async-signal-safe (unless the assertion fails, of course). 3) add NM_MORE_ASSERTS_EFFECTIVE. When using no glib (libnm-std-aux), the assert is implemented by C89 assert(), while libnm-glib-aux redirects that to g_assert(). Note that these assertions are only in effect, if both NM_MORE_ASSERTS and NDEBUG/G_DISABLE_ASSERT say so. NM_MORE_ASSERTS_EFFECTIVE is thus the effectively used level for nm_assert(). 4) use the proper __assert_fail() and g_assertion_message_expr() calls. __assert_fail() is not standard, but it is there for glibc and musl. So relying on this is probably fine. Otherwise, we will get a compilation error and notice it. --- src/libnm-glib-aux/nm-macros-internal.h | 16 ++-- src/libnm-std-aux/nm-std-aux.h | 97 ++++++++++++++----------- 2 files changed, 64 insertions(+), 49 deletions(-) diff --git a/src/libnm-glib-aux/nm-macros-internal.h b/src/libnm-glib-aux/nm-macros-internal.h index 220c1bb492..60f8aa0ee6 100644 --- a/src/libnm-glib-aux/nm-macros-internal.h +++ b/src/libnm-glib-aux/nm-macros-internal.h @@ -525,16 +525,22 @@ nm_str_realloc(char *str) /*****************************************************************************/ /* redefine assertions to use g_assert*() */ -#undef _nm_assert_call -#undef _nm_assert_call_not_reached -#define _nm_assert_call(cond) g_assert(cond) -#define _nm_assert_call_not_reached() g_assert_not_reached() +#undef _nm_assert_fail +#define _nm_assert_fail(msg) \ + g_assertion_message_expr(G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, msg) + +#undef _NM_ASSERT_FAIL_ENABLED +#ifndef G_DISABLE_ASSERT +#define _NM_ASSERT_FAIL_ENABLED 1 +#else +#define _NM_ASSERT_FAIL_ENABLED 0 +#endif /* Usage: * * if (NM_MORE_ASSERT_ONCE (5)) { extra_check (); } * - * This will only run the check once, and only if NM_MORE_ASSERT is >= than + * This will only run the check once, and only if NM_MORE_ASSERTS is >= than * more_assert_level. */ #define NM_MORE_ASSERT_ONCE(more_assert_level) \ diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index aad8931207..320da0b111 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -194,60 +194,69 @@ typedef uint64_t _nm_bitwise nm_be64_t; #define NM_MORE_ASSERTS 0 #endif -#ifndef _nm_assert_call -#define _nm_assert_call(cond) assert(cond) -#define _nm_assert_call_not_reached() assert(0) +#define _nm_assert_fail(msg) __assert_fail((msg), __FILE__, __LINE__, __func__) +#ifndef NDEBUG +#define _NM_ASSERT_FAIL_ENABLED 1 +#else +#define _NM_ASSERT_FAIL_ENABLED 1 #endif -#if NM_MORE_ASSERTS -#define nm_assert(cond) \ - ({ \ - _nm_assert_call(cond); \ - 1; \ +#define NM_MORE_ASSERTS_EFFECTIVE (_NM_ASSERT_FAIL_ENABLED ? NM_MORE_ASSERTS : 0) + +#define nm_assert(cond) \ + ({ \ + /* nm_assert() must do *nothing* of effect, except evaluating + * @cond (0 or 1 times). + * + * As such, nm_assert() is async-signal-safe (provided @cond is, and + * the assertion does not fail). */ \ + if (NM_MORE_ASSERTS_EFFECTIVE == 0) { \ + /* pass */ \ + } else if NM_LIKELY (cond) { \ + /* pass */ \ + } else { \ + _nm_assert_fail(#cond); \ + } \ + 1; \ }) -#define nm_assert_se(cond) \ - ({ \ - if (NM_LIKELY(cond)) { \ - ; \ - } else { \ - _nm_assert_call(0 && (cond)); \ - } \ - 1; \ + +#define nm_assert_se(cond) \ + ({ \ + /* nm_assert() must do *nothing* of effect, except evaluating + * @cond (exactly 1 times). + * + * As such, nm_assert() is async-signal-safe (provided @cond is, and + * the assertion does not fail). */ \ + if NM_LIKELY (cond) { \ + /* pass */ \ + } else if (NM_MORE_ASSERTS_EFFECTIVE == 0) { \ + /* pass */ \ + } else { \ + _nm_assert_fail(#cond); \ + } \ + 1; \ }) -#define nm_assert_not_reached() \ - ({ \ - _nm_assert_call_not_reached(); \ - 1; \ + +#define nm_assert_not_reached() \ + ({ \ + _nm_assert_fail("unreachable"); \ + 1; \ }) -#else -#define nm_assert(cond) \ - ({ \ - if (0) { \ - if (cond) {} \ - } \ - 1; \ - }) -#define nm_assert_se(cond) \ - ({ \ - if (NM_LIKELY(cond)) { \ - ; \ - } \ - 1; \ - }) -#define nm_assert_not_reached() ({ 1; }) -#endif /* This is similar nm_assert_not_reached(), but it's supposed to be used only during * development. Like _XXX_ comments, they can be used as a marker that something still * needs to be done. */ -#define XXX(msg) \ - nm_assert(!"X" \ - "XX error: " msg "") +#define XXX(msg) \ + ({ \ + _nm_assert_fail("X" \ + "XX error: " msg ""); \ + 1; \ + }) -#define nm_assert_unreachable_val(val) \ - ({ \ - nm_assert_not_reached(); \ - (val); \ +#define nm_assert_unreachable_val(val) \ + ({ \ + _nm_assert_fail("unreachable value " #val); \ + (val); \ }) #define NM_STATIC_ASSERT(cond) static_assert(cond, "") From 1ce29e120bfc763441495bbf5620fa0751753c9c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Oct 2022 10:15:10 +0200 Subject: [PATCH 4/8] std-aux: drop assertion and function name from assert() in release mode For g_assert() and g_return*() we already do the same, in "src/libnm-glib-aux/nm-gassert-patch.h" Also patch __assert_fail() so that it omits the condition text and the function name in production builds. Note that this is a bit ugly, for two reasons: - again, we make assumptions that __assert_fail() exists. In practice, this is the case for glibc and musl. - can be included multiple times, while also forward declaring __assert_fail(). That means, we cannot add a macro #define __assert_fail(...) because that would break the forward declaration. Instead, just `#define __assert_fail _nm_assert_fail_internal` Of course, this only affects direct calls to assert(), which we have few. nm_assert() is not affected, because that anyway doesn't do anything, unless NM_MORE_ASSERTS is enabled. --- .../tests/test-shared-general.c | 3 +++ src/libnm-std-aux/nm-std-aux.h | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/libnm-glib-aux/tests/test-shared-general.c b/src/libnm-glib-aux/tests/test-shared-general.c index 4131c891aa..dc04c3d5c0 100644 --- a/src/libnm-glib-aux/tests/test-shared-general.c +++ b/src/libnm-glib-aux/tests/test-shared-general.c @@ -78,6 +78,9 @@ test_gpid(void) * the case. */ int_ptr = &pid; g_assert_cmpint(*int_ptr, ==, 42); + + /* also check how assert() works. */ + assert(*int_ptr == 42); } /*****************************************************************************/ diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index 320da0b111..526b596e63 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -194,6 +194,25 @@ typedef uint64_t _nm_bitwise nm_be64_t; #define NM_MORE_ASSERTS 0 #endif +#if NM_MORE_ASSERTS == 0 +/* The string with the assertion check and the function name blows up the + * binary size. In production mode, let's drop those, similar to + * g_assertion_message_expr. + * + * Note that can be included multiple times. We can thus + * not redefine __assert_fail(...). Instead, just redefine the name + * __assert_fail. */ +_nm_noreturn static inline void +_nm_assert_fail_internal(const char *assertion, + const char *file, + unsigned int line, + const char *function) +{ + __assert_fail("", file, line, ""); +} +#define __assert_fail _nm_assert_fail_internal +#endif + #define _nm_assert_fail(msg) __assert_fail((msg), __FILE__, __LINE__, __func__) #ifndef NDEBUG #define _NM_ASSERT_FAIL_ENABLED 1 From ad7d5887cdafeef85e451f5866f4e083196be0c0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Oct 2022 20:15:46 +0200 Subject: [PATCH 5/8] all: cleanup close() handling and clarify nm_close()/nm_close_with_error() Cleanup the handling of close(). First of all, closing an invalid (non-negative) file descriptor (EBADF) is always a serious bug. We want to catch that. Hence, we should use nm_close() (or nm_close_with_error()) which asserts against such bugs. Don't ever use close() directly, to get that additional assertion. Also, our nm_close() handles EINTR internally and correctly. Recent POSIX defines that on EINTR the close should be retried. On Linux, that is never correct. After close() returns, the file descriptor is always closed (or invalid). nm_close() gets this right, and pretends that EINTR is a success (without retrying). The majority of our file descriptors are sockets, etc. That means, often an error from close isn't something that we want to handle. Adjust nm_close() to return no error and preserve the caller's errno. That is the appropriate reaction to error (ignoring it) in most of our cases. And error from close may mean that there was an IO error (except EINTR and EBADF). In a few cases, we may want to handle that. For those cases we have nm_close_with_error(). TL;DR: use almost always nm_close(). Unless you want to handle the error code, then use nm_close_with_error(). Never use close() directly. There is much reading on the internet about handling errors of close and in particular EINTR. See the following links: https://lwn.net/Articles/576478/ https://askcodes.net/coding/what-to-do-if-a-posix-close-call-fails- https://www.austingroupbugs.net/view.php?id=529 https://sourceware.org/bugzilla/show_bug.cgi?id=14627 https://news.ycombinator.com/item?id=3363819 https://peps.python.org/pep-0475/ --- contrib/scripts/checkpatch.pl | 1 + src/core/main-utils.c | 7 +- src/libnm-platform/nm-linux-platform.c | 23 ++-- src/libnm-platform/wifi/nm-wifi-utils-wext.c | 2 +- src/libnm-std-aux/nm-std-aux.h | 123 +++++++++++++------ 5 files changed, 103 insertions(+), 53 deletions(-) diff --git a/contrib/scripts/checkpatch.pl b/contrib/scripts/checkpatch.pl index 3c53ffae06..797caac847 100755 --- a/contrib/scripts/checkpatch.pl +++ b/contrib/scripts/checkpatch.pl @@ -206,6 +206,7 @@ complain ("Define setting properties with _nm_setting_property_define_direct_*() complain ("Use nm_g_array_{index,first,last,index_p}() instead of g_array_index(), as it nm_assert()s for valid element size and out-of-bound access") if $line =~ /\bg_array_index\b/; complain ("Use spaces instead of tabs") if $line =~ /\t/; complain ("Prefer implementing private pointers via _NM_GET_PRIVATE() or _NM_GET_PRIVATE_PTR() (the latter, if the private data has an opqaue pointer in the header file)") if $line =~ /\b(g_type_class_add_private|G_TYPE_INSTANCE_GET_PRIVATE)\b/; +complain ("Don't use close()/g_close(). Instead, use nm_close() (or nm_close_with_error()).") if $line =~ /\b(close|g_close)\b *\(/; complain ("Use nm_memdup() instead of g_memdup(). The latter has a size argument of type guint") if $line =~ /\bg_memdup\b/; # Further on we process stuff without comments. diff --git a/src/core/main-utils.c b/src/core/main-utils.c index aec73e12db..b18ae311ed 100644 --- a/src/core/main-utils.c +++ b/src/core/main-utils.c @@ -80,6 +80,7 @@ nm_main_utils_write_pidfile(const char *pidfile) int fd; int errsv; gboolean success = FALSE; + int r; if ((fd = open(pidfile, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 00644)) < 0) { errsv = errno; @@ -94,9 +95,9 @@ nm_main_utils_write_pidfile(const char *pidfile) } else success = TRUE; - if (nm_close(fd)) { - errsv = errno; - fprintf(stderr, _("Closing %s failed: %s\n"), pidfile, nm_strerror_native(errsv)); + r = nm_close_with_error(fd); + if (r < 0) { + fprintf(stderr, _("Closing %s failed: %s\n"), pidfile, nm_strerror_native(-r)); } return success; diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index d2d48bb99a..ac137545c7 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -5608,12 +5608,14 @@ sysctl_set_internal(NMPlatform *platform, const char *path, const char *value) { - int fd, tries; + int fd; + int tries; gssize nwrote; gssize len; char *actual; gs_free char *actual_free = NULL; int errsv; + int r; if (dirfd < 0) { pathid = path; @@ -5707,18 +5709,13 @@ sysctl_set_internal(NMPlatform *platform, _LOGE("sysctl: failed to set '%s' to '%s' after three attempts", path, value); } - if (nwrote < len - 1) { - if (nm_close(fd) != 0) { - if (errsv != 0) - errno = errsv; - } else if (errsv != 0) - errno = errsv; - else - errno = EIO; - return FALSE; - } - if (nm_close(fd) != 0) { - /* errno is already properly set. */ + r = nm_close_with_error(fd); + if (r < 0 || nwrote < len - 1) { + if (errsv == 0) { + /* propagate the error from nm_close_with_error(). */ + errsv = (r < 0) ? -r : EIO; + } + errno = errsv; return FALSE; } diff --git a/src/libnm-platform/wifi/nm-wifi-utils-wext.c b/src/libnm-platform/wifi/nm-wifi-utils-wext.c index 678d71fe60..eac3c929bc 100644 --- a/src/libnm-platform/wifi/nm-wifi-utils-wext.c +++ b/src/libnm-platform/wifi/nm-wifi-utils-wext.c @@ -90,7 +90,7 @@ dispose(GObject *object) { NMWifiUtilsWext *wext = NM_WIFI_UTILS_WEXT(object); - wext->fd = nm_close(wext->fd); + nm_clear_fd(&wext->fd); } static gboolean diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index 526b596e63..fbfa619980 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -957,38 +957,6 @@ _NM_IN_STRSET_EVAL_op_streq(const char *x1, const char *x) /*****************************************************************************/ -/** - * nm_close: - * - * Like close() but throws an assertion if the input fd is - * invalid. Closing an invalid fd is a programming error, so - * it's better to catch it early. - */ -static inline int -nm_close(int fd) -{ - int r; - - r = close(fd); - nm_assert(r != -1 || fd < 0 || errno != EBADF); - return r; -} - -static inline bool -nm_clear_fd(int *p_fd) -{ - int fd; - - if (!p_fd || (fd = *p_fd) < 0) - return false; - - *p_fd = -1; - nm_close(fd); - return true; -} - -/*****************************************************************************/ - /* Note: @value is only evaluated when *out_val is present. * Thus, * NM_SET_OUT (out_str, g_strdup ("hallo")); @@ -1066,13 +1034,96 @@ _nm_auto_protect_errno(const int *p_saved_errno) /*****************************************************************************/ +/** + * nm_close_with_error: + * + * Wrapper around close(). + * + * This fails an nm_assert() for EBADF with a non-negative file descriptor. Trying + * to close an invalid file descriptor is always a serious bug. Never use close() + * directly, because we want to catch such bugs. + * + * This also suppresses any EINTR and pretends success. That is appropriate + * on Linux (but not necessarily on other POSIX systems). + * + * In no case is it appropriate to use @fd afterwards (or retry). + * + * This function returns 0 on success, or a negative errno value. + * On success, errno is undefined afterwards. On failure, errno is + * the same as the (negative) return value. + * + * In the common case, when you don't intend to handle the error from close(), + * prefer nm_close() over nm_close_with_error(). Never use close() directly. + * + * The function is also async-signal-safe (unless an assertion fails). + * + * Returns: 0 on success or the negative errno from close(). + */ +static inline int +nm_close_with_error(int fd) +{ + int r; + + r = close(fd); + + if (r != 0) { + int errsv = errno; + + nm_assert(r == -1); + nm_assert((fd < 0 && errsv == EBADF) || (fd >= 0 && errsv != EBADF)); + + if (errsv == EINTR) { + /* There isn't really much we can do about EINTR. On Linux, always this means + * the FD was closed. On some POSIX systems that may be different and retry + * would be appropriate. + * + * Whether there was any IO error is unknown. Assume not and signal success. */ + return 0; + } + + return -errsv; + } + + return 0; +} + +/** + * nm_close: + * + * Wrapper around nm_close_with_error(), which ignores any error and preserves the + * caller's errno. + * + * We usually don't care about errors from close, so this is usually preferable over + * nm_close_with_error(). Never use close() directly. + * + * Everything from nm_close_with_error() applies. + */ +static inline void +nm_close(int fd) +{ + NM_AUTO_PROTECT_ERRNO(errsv); + + nm_close_with_error(fd); +} + +static inline bool +nm_clear_fd(int *p_fd) +{ + int fd; + + if (!p_fd || (fd = *p_fd) < 0) + return false; + + *p_fd = -1; + nm_close(fd); + return true; +} + static inline void _nm_auto_close(int *pfd) { - if (*pfd >= 0) { - NM_AUTO_PROTECT_ERRNO(errsv); - (void) nm_close(*pfd); - } + if (*pfd >= 0) + nm_close(*pfd); } #define nm_auto_close nm_auto(_nm_auto_close) From 3254f33b330e6f90c9e8a70757cdfa9c1c38d0d1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Oct 2022 10:33:39 +0200 Subject: [PATCH 6/8] std-aux: assert against any EBADF error in nm_close() Previously, nm_close() accepted EBADF, as long as fd was negative. Getting EBADF for a non-negative file descriptor is a serious bug, because in multi threaded applications there is a race to close an unrelated file descriptor. Also, it indicates that somebody messed up the tracking of the resource, which indicates a bug. Getting EBADF on a negative FD is less of a problem. But it still indicates that the caller does something they likely don't intend to. Assert against any EBADF. Note that nm_clear_fd() and nm_auto_close take already care to not close negative "file descriptors". So, if you use those, you are not affected. If you want a close function that doesn't care about whether fd is set, then maybe use `nm_clear_fd(&fd)` instead. --- src/libnm-std-aux/nm-std-aux.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index fbfa619980..4f307ebf40 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -1070,7 +1070,17 @@ nm_close_with_error(int fd) int errsv = errno; nm_assert(r == -1); - nm_assert((fd < 0 && errsv == EBADF) || (fd >= 0 && errsv != EBADF)); + + /* EBADF indicates a bug. + * + * - if fd is non-negative, this means the tracking of the descriptor + * got messed up. That's very bad, somebody closed a wrong FD or we + * might do so. On a multi threaded application, messing up the tracking + * of the file descriptor means we race against closing an unrelated FD. + * - if fd is negative, it may not be a bug but intentional. However, our callers + * are not supposed to call close() on a negative FD either. Assert + * against that too. */ + nm_assert(errsv != EBADF); if (errsv == EINTR) { /* There isn't really much we can do about EINTR. On Linux, always this means From 70417fda9ec601327ff654400ffcac9fce6c93e0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Oct 2022 12:02:45 +0200 Subject: [PATCH 7/8] glib-aux: handle errors from close() in nm_utils_file_set_contents() glib's g_file_set_contents() also does that. --- src/libnm-glib-aux/nm-io-utils.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-io-utils.c b/src/libnm-glib-aux/nm-io-utils.c index b4bf30a8d4..d143059561 100644 --- a/src/libnm-glib-aux/nm-io-utils.c +++ b/src/libnm-glib-aux/nm-io-utils.c @@ -352,6 +352,7 @@ nm_utils_file_set_contents(const char *filename, int errsv; gssize s; int fd; + int r; g_return_val_if_fail(filename, FALSE); g_return_val_if_fail(contents || !length, FALSE); @@ -415,7 +416,16 @@ nm_utils_file_set_contents(const char *filename, tmp_name); } - nm_close(fd); + r = nm_close_with_error(fd); + if (r < 0) { + errsv = NM_ERRNO_NATIVE(-r); + unlink(tmp_name); + return _get_contents_error(error, + errsv, + out_errsv, + "failed close() after writing file %s", + tmp_name); + } if (rename(tmp_name, filename)) { errsv = NM_ERRNO_NATIVE(errno); From d98553e9e78bc3a1fd997caeb3cf92cded7b775a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Oct 2022 11:51:11 +0200 Subject: [PATCH 8/8] main: use helper function to write pid file in nm_main_utils_write_pidfile() On the surface, writing a file seams simple enough. But there are many pitfalls: - we should retry on EINTR. - we should check for incomplete writes and loop. - we possibly should check errors from close. - we possibly should write to a temporary file and do atomic rename. Use nm_utils_file_set_contents() to get this right. --- src/core/main-utils.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/core/main-utils.c b/src/core/main-utils.c index b18ae311ed..c6fa05c0ca 100644 --- a/src/core/main-utils.c +++ b/src/core/main-utils.c @@ -18,6 +18,7 @@ #include "main-utils.h" #include "NetworkManagerUtils.h" #include "nm-config.h" +#include "libnm-glib-aux/nm-io-utils.h" static gboolean sighup_handler(gpointer user_data) @@ -76,31 +77,16 @@ nm_main_utils_setup_signals(GMainLoop *main_loop) gboolean nm_main_utils_write_pidfile(const char *pidfile) { - char pid[16]; - int fd; - int errsv; - gboolean success = FALSE; - int r; + gs_free_error GError *error = NULL; + char pid[16]; - if ((fd = open(pidfile, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 00644)) < 0) { - errsv = errno; - fprintf(stderr, _("Opening %s failed: %s\n"), pidfile, nm_strerror_native(errsv)); + nm_sprintf_buf(pid, "%lld", (long long) getpid()); + if (!nm_utils_file_set_contents(pidfile, pid, -1, 00644, NULL, NULL, &error)) { + fprintf(stderr, _("Writing to %s failed: %s\n"), pidfile, error->message); return FALSE; } - g_snprintf(pid, sizeof(pid), "%d", getpid()); - if (write(fd, pid, strlen(pid)) < 0) { - errsv = errno; - fprintf(stderr, _("Writing to %s failed: %s\n"), pidfile, nm_strerror_native(errsv)); - } else - success = TRUE; - - r = nm_close_with_error(fd); - if (r < 0) { - fprintf(stderr, _("Closing %s failed: %s\n"), pidfile, nm_strerror_native(-r)); - } - - return success; + return TRUE; } void