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..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,30 +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; + 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; - - if (nm_close(fd)) { - errsv = errno; - fprintf(stderr, _("Closing %s failed: %s\n"), pidfile, nm_strerror_native(errsv)); - } - - return success; + return TRUE; } void 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); 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-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 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-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 96e379926f..4f307ebf40 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))) @@ -193,60 +194,88 @@ 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) +#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 -#if NM_MORE_ASSERTS -#define nm_assert(cond) \ - ({ \ - _nm_assert_call(cond); \ - 1; \ - }) -#define nm_assert_se(cond) \ - ({ \ - if (NM_LIKELY(cond)) { \ - ; \ - } else { \ - _nm_assert_call(0 && (cond)); \ - } \ - 1; \ - }) -#define nm_assert_not_reached() \ - ({ \ - _nm_assert_call_not_reached(); \ - 1; \ - }) +#define _nm_assert_fail(msg) __assert_fail((msg), __FILE__, __LINE__, __func__) +#ifndef NDEBUG +#define _NM_ASSERT_FAIL_ENABLED 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; }) +#define _NM_ASSERT_FAIL_ENABLED 1 #endif +#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) \ + ({ \ + /* 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_fail("unreachable"); \ + 1; \ + }) + /* 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, "") @@ -928,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")); @@ -1037,13 +1034,106 @@ _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); + + /* 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 + * 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)