merge: branch 'bg/close-rh1451236'

https://bugzilla.redhat.com/show_bug.cgi?id=1451236

12 files changed, 76 insertions, 41 deletions

diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h
index bbdf50be71..5376e615ec 100644
--- a/shared/nm-utils/nm-macros-internal.h
+++ b/shared/nm-utils/nm-macros-internal.h
@@ -59,6 +59,8 @@
 
 #define nm_auto(fcn) __attribute__ ((cleanup(fcn)))
 
+static inline int nm_close (int fd);
+
 /**
  * nm_auto_free:
  *
@@ -96,7 +98,7 @@ _nm_auto_close_impl (int *pfd)
 	if (*pfd >= 0) {
 		int errsv = errno;
 
-		(void) close (*pfd);
+		(void) nm_close (*pfd);
 		errno = errsv;
 	}
 }
@@ -1149,4 +1151,35 @@ nm_decode_version (guint version, guint *major, guint *minor, guint *micro)
 
 /*****************************************************************************/
 
+static inline int
+nm_steal_fd (int *p_fd)
+{
+	int fd;
+
+	if (   p_fd
+	    && ((fd = *p_fd) > 0)) {
+		*p_fd = -1;
+		return fd;
+	}
+	return -1;
+}
+
+/**
+ * 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)
+{
+	if (fd >= 0) {
+		if (close (fd) == 0)
+			return 0;
+		nm_assert (errno != EBADF);
+	}
+	return -1;
+}
+
 #endif /* __NM_MACROS_INTERNAL_H__ */
diff --git a/src/devices/adsl/nm-device-adsl.c b/src/devices/adsl/nm-device-adsl.c
index feeb243d02..e9bd41ae00 100644
--- a/src/devices/adsl/nm-device-adsl.c
+++ b/src/devices/adsl/nm-device-adsl.c
@@ -227,7 +227,7 @@ br2684_assign_vcc (NMDeviceAdsl *self, NMSettingAdsl *s_adsl)
 	return TRUE;
 
 error:
-	close (priv->brfd);
+	nm_close (priv->brfd);
 	priv->brfd = -1;
 	return FALSE;
 }
@@ -519,10 +519,8 @@ adsl_cleanup (NMDeviceAdsl *self)
 
 	g_signal_handlers_disconnect_by_func (nm_device_get_platform (NM_DEVICE (self)), G_CALLBACK (link_changed_cb), self);
 
-	if (priv->brfd >= 0) {
-		close (priv->brfd);
-		priv->brfd = -1;
-	}
+	nm_close (priv->brfd);
+	priv->brfd = -1;
 
 	nm_clear_g_source (&priv->nas_update_id);
 
diff --git a/src/devices/bluetooth/nm-bluez5-dun.c b/src/devices/bluetooth/nm-bluez5-dun.c
index aba3a0dd97..ca09b276e7 100644
--- a/src/devices/bluetooth/nm-bluez5-dun.c
+++ b/src/devices/bluetooth/nm-bluez5-dun.c
@@ -386,11 +386,11 @@ nm_bluez5_dun_cleanup (NMBluez5DunContext *context)
 			ioctl (context->rfcomm_fd, RFCOMMRELEASEDEV, &req);
 			context->rfcomm_id = -1;
 		}
-		close (context->rfcomm_fd);
+		nm_close (context->rfcomm_fd);
 		context->rfcomm_fd = -1;
 	}
 
-	close (context->rfcomm_tty_fd);
+	nm_close (context->rfcomm_tty_fd);
 	context->rfcomm_tty_fd = -1;
 }
 
diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c
index 4bce0df4ea..d5392b7d41 100644
--- a/src/dns/nm-dns-manager.c
+++ b/src/dns/nm-dns-manager.c
@@ -497,7 +497,7 @@ dispatch_netconfig (NMDnsManager *self,
 		g_free (str);
 	}
 
-	close (fd);
+	nm_close (fd);
 
 	/* Wait until the process exits */
 	if (!nm_utils_kill_child_sync (pid, 0, LOGD_DNS, "netconfig", &status, 1000, 0)) {
@@ -1633,7 +1633,7 @@ _check_resconf_immutable (NMDnsManagerResolvConfManager rc_manager)
 		if (fd != -1) {
 			if (ioctl (fd, FS_IOC_GETFLAGS, &flags) != -1)
 				immutable = NM_FLAGS_HAS (flags, FS_IMMUTABLE_FL);
-			close (fd);
+			nm_close (fd);
 		}
 		return immutable ? NM_DNS_MANAGER_RESOLV_CONF_MAN_IMMUTABLE : rc_manager;
 	}
diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c
index 026c729b4b..138d871717 100644
--- a/src/nm-core-utils.c
+++ b/src/nm-core-utils.c
@@ -2716,6 +2716,8 @@ _get_contents_error (GError **error, int errsv, const char *format, ...)
  * nm_utils_fd_get_contents:
  * @fd: open file descriptor to read. The fd will not be closed,
  *   but don't rely on it's state afterwards.
+ * @close_fd: if %TRUE, @fd will be closed by the function.
+ *  Passing %TRUE here might safe a syscall for dup().
  * @max_length: allocate at most @max_length bytes. If the
  *   file is larger, reading will fail. Set to zero to use
  *   a very large default.
@@ -2743,11 +2745,13 @@ _get_contents_error (GError **error, int errsv, const char *format, ...)
  */
 int
 nm_utils_fd_get_contents (int fd,
+                          gboolean close_fd,
                           gsize max_length,
                           char **contents,
                           gsize *length,
                           GError **error)
 {
+	nm_auto_close int fd_keeper = close_fd ? fd : -1;
 	struct stat stat_buf;
 	gs_free char *str = NULL;
 
@@ -2795,9 +2799,13 @@ nm_utils_fd_get_contents (int fd,
 		gsize n_have, n_alloc;
 		int fd2;
 
-		fd2 = dup (fd);
-		if (fd2 < 0)
-			return _get_contents_error (error, 0, "error during dup");
+		if (close_fd)
+			fd2 = nm_steal_fd (&fd_keeper);
+		else {
+			fd2 = dup (fd);
+			if (fd2 < 0)
+				return _get_contents_error (error, 0, "error during dup");
+		}
 
 		if (!(f = fdopen (fd2, "r"))) {
 			close (fd2);
@@ -2892,7 +2900,7 @@ nm_utils_file_get_contents (int dirfd,
                             gsize *length,
                             GError **error)
 {
-	nm_auto_close int fd = -1;
+	int fd;
 	int errsv;
 
 	g_return_val_if_fail (filename && filename[0], -EINVAL);
@@ -2925,6 +2933,7 @@ nm_utils_file_get_contents (int dirfd,
 		}
 	}
 	return nm_utils_fd_get_contents (fd,
+	                                 TRUE,
 	                                 max_length,
 	                                 contents,
 	                                 length,
diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h
index 8dc65283ed..2883199884 100644
--- a/src/nm-core-utils.h
+++ b/src/nm-core-utils.h
@@ -254,6 +254,7 @@ const char *nm_utils_ip4_property_path (const char *ifname, const char *property
 gboolean nm_utils_is_specific_hostname (const char *name);
 
 int nm_utils_fd_get_contents (int fd,
+                              gboolean close_fd,
                               gsize max_length,
                               char **contents,
                               gsize *length,
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index c40a2c532b..440d2760cb 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -3019,7 +3019,7 @@ sysctl_set (NMPlatform *platform, const char *pathid, int dirfd, const char *pat
 	}
 
 	if (nwrote < len - 1) {
-		if (close (fd) != 0) {
+		if (nm_close (fd) != 0) {
 			if (errsv != 0)
 				errno = errsv;
 		} else if (errsv != 0)
@@ -3028,7 +3028,7 @@ sysctl_set (NMPlatform *platform, const char *pathid, int dirfd, const char *pat
 			errno = EIO;
 		return FALSE;
 	}
-	if (close (fd) != 0) {
+	if (nm_close (fd) != 0) {
 		/* errno is already properly set. */
 		return FALSE;
 	}
@@ -5601,26 +5601,26 @@ tun_add (NMPlatform *platform, const char *name, gboolean tap,
 		ifr.ifr_flags |= NM_IFF_MULTI_QUEUE;
 
 	if (ioctl (fd, TUNSETIFF, &ifr)) {
-		close (fd);
+		nm_close (fd);
 		return FALSE;
 	}
 
 	if (owner >= 0 && owner < G_MAXINT32) {
 		if (ioctl (fd, TUNSETOWNER, (uid_t) owner)) {
-			close (fd);
+			nm_close (fd);
 			return FALSE;
 		}
 	}
 
 	if (group >= 0 && group < G_MAXINT32) {
 		if (ioctl (fd, TUNSETGROUP, (gid_t) group)) {
-			close (fd);
+			nm_close (fd);
 			return FALSE;
 		}
 	}
 
 	if (ioctl (fd, TUNSETPERSIST, 1)) {
-		close (fd);
+		nm_close (fd);
 		return FALSE;
 	}
 	do_request_link (platform, 0, name);
diff --git a/src/platform/nmp-netns.c b/src/platform/nmp-netns.c
index 4acd47617c..34215828ce 100644
--- a/src/platform/nmp-netns.c
+++ b/src/platform/nmp-netns.c
@@ -299,7 +299,7 @@ _netns_new (GError **error)
 		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
 		             "Failed opening mntns: %s",
 		             g_strerror (errsv));
-		close (fd_net);
+		nm_close (fd_net);
 		return NULL;
 	}
 
@@ -620,7 +620,7 @@ nmp_netns_bind_to_path (NMPNetns *self, const char *filename, int *out_fd)
 		       filename, g_strerror (errsv));
 		return FALSE;
 	}
-	close (fd);
+	nm_close (fd);
 
 	if (mount (PROC_SELF_NS_NET, filename, "none", MS_BIND, NULL) != 0) {
 		errsv = errno;
@@ -702,15 +702,11 @@ dispose (GObject *object)
 	NMPNetns *self = NMP_NETNS (object);
 	NMPNetnsPrivate *priv = NMP_NETNS_GET_PRIVATE (self);
 
-	if (priv->fd_net > 0) {
-		close (priv->fd_net);
-		priv->fd_net = 0;
-	}
+	nm_close (priv->fd_net);
+	priv->fd_net = -1;
 
-	if (priv->fd_mnt > 0) {
-		close (priv->fd_mnt);
-		priv->fd_mnt = 0;
-	}
+	nm_close (priv->fd_mnt);
+	priv->fd_mnt = -1;
 
 	G_OBJECT_CLASS (nmp_netns_parent_class)->dispose (object);
 }
diff --git a/src/platform/wifi/wifi-utils-wext.c b/src/platform/wifi/wifi-utils-wext.c
index 1bc29ae8c3..c4d3c99962 100644
--- a/src/platform/wifi/wifi-utils-wext.c
+++ b/src/platform/wifi/wifi-utils-wext.c
@@ -97,8 +97,7 @@ wifi_wext_deinit (WifiData *parent)
 {
 	WifiDataWext *wext = (WifiDataWext *) parent;
 
-	if (wext->fd >= 0)
-		close (wext->fd);
+	nm_close (wext->fd);
 }
 
 static gboolean
@@ -757,7 +756,7 @@ wifi_wext_is_wifi (const char *iface)
 		nm_utils_ifname_cpy (iwr.ifr_ifrn.ifrn_name, iface);
 		if (ioctl (fd, SIOCGIWNAME, &iwr) == 0)
 			is_wifi = TRUE;
-		close (fd);
+		nm_close (fd);
 	}
 	return is_wifi;
 }
diff --git a/src/ppp/nm-ppp-manager.c b/src/ppp/nm-ppp-manager.c
index 75299d1a66..3ef3f3dc84 100644
--- a/src/ppp/nm-ppp-manager.c
+++ b/src/ppp/nm-ppp-manager.c
@@ -1088,7 +1088,7 @@ _ppp_cleanup (NMPPPManager *manager)
 	if (priv->monitor_fd >= 0) {
 		/* Get the stats one last time */
 		monitor_cb (manager);
-		close (priv->monitor_fd);
+		nm_close (priv->monitor_fd);
 		priv->monitor_fd = -1;
 	}
 
diff --git a/src/settings/nm-inotify-helper.c b/src/settings/nm-inotify-helper.c
index a0432a25c0..4c65b02da5 100644
--- a/src/settings/nm-inotify-helper.c
+++ b/src/settings/nm-inotify-helper.c
@@ -188,8 +188,7 @@ finalize (GObject *object)
 {
 	NMInotifyHelperPrivate *priv = NM_INOTIFY_HELPER_GET_PRIVATE ((NMInotifyHelper *) object);
 
-	if (priv->ifd >= 0)
-		close (priv->ifd);
+	nm_close (priv->ifd);
 
 	g_hash_table_destroy (priv->wd_refs);
 
diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c
index 87fefd8db6..911da73a05 100644
--- a/src/settings/plugins/ifcfg-rh/shvar.c
+++ b/src/settings/plugins/ifcfg-rh/shvar.c
@@ -818,7 +818,8 @@ svOpenFileInternal (const char *name, gboolean create, GError **error)
 		return NULL;
 	}
 
-	if (nm_utils_fd_get_contents (fd,
+	if (nm_utils_fd_get_contents (closefd ? nm_steal_fd (&fd) : fd,
+	                              closefd,
 	                              10 * 1024 * 1024,
 	                              &arena,
 	                              NULL,
@@ -842,8 +843,8 @@ svOpenFileInternal (const char *name, gboolean create, GError **error)
 	/* closefd is set if we opened the file read-only, so go ahead and
 	 * close it, because we can't write to it anyway */
 	if (!closefd) {
-		s->fd = fd;
-		fd = -1;
+		nm_assert (fd > 0);
+		s->fd = nm_steal_fd (&fd);
 	}
 
 	return s;
@@ -1326,8 +1327,7 @@ svCloseFile (shvarFile *s)
 
 	g_return_if_fail (s != NULL);
 
-	if (s->fd != -1)
-		close (s->fd);
+	nm_close (s->fd);
 	g_free (s->fileName);
 	c_list_for_each_safe (current, safe, &s->lst_head)
 		line_free (c_list_entry (current, shvarLine, lst));