From ad7d5887cdafeef85e451f5866f4e083196be0c0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Oct 2022 20:15:46 +0200 Subject: [PATCH] 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 3c53ffae0..797caac84 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 aec73e12d..b18ae311e 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 d2d48bb99..ac137545c 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 678d71fe6..eac3c929b 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 526b596e6..fbfa61998 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)