libnm-core/utils: update hwaddr utilities

_nm_utils_hwaddr_length() did a validation of the string
and returned the length of the address. In all cases where
we were interested in that, we also either want to validate
the address, get the address in binary form, or canonicalize
the address.

We can avoid these duplicate checks, by using _nm_utils_hwaddr_aton()
which both does the parsing and returning the length.

4 files changed, 205 insertions, 160 deletions

diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h
index 0ea59e4f08..8fba357167 100644
--- a/libnm-core/nm-core-internal.h
+++ b/libnm-core/nm-core-internal.h
@@ -124,7 +124,10 @@ guint32 _nm_setting_get_setting_priority (NMSetting *setting);
 
 gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue *value);
 
-guint _nm_utils_hwaddr_length (const char *asc);
+#define NM_UTILS_HWADDR_LEN_MAX_STR (NM_UTILS_HWADDR_LEN_MAX * 3)
+
+guint8 *_nm_utils_hwaddr_aton (const char *asc, gpointer buffer, gsize buffer_length, gsize *out_length);
+const char *nm_utils_hwaddr_ntoa_buf (gconstpointer addr, gsize addr_len, gboolean upper_case, char *buf, gsize buf_len);
 
 char *_nm_utils_bin2str (gconstpointer addr, gsize length, gboolean upper_case);
 
diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index d963323e22..4be430af6b 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2995,17 +2995,64 @@ nm_utils_wifi_strength_bars (guint8 strength)
 gsize
 nm_utils_hwaddr_len (int type)
 {
-	g_return_val_if_fail (type == ARPHRD_ETHER || type == ARPHRD_INFINIBAND, 0);
-
 	if (type == ARPHRD_ETHER)
 		return ETH_ALEN;
 	else if (type == ARPHRD_INFINIBAND)
 		return INFINIBAND_ALEN;
 
-	g_assert_not_reached ();
+	g_return_val_if_reached (0);
 }
 
-#define HEXVAL(c) ((c) <= '9' ? (c) - '0' : ((c) & 0x4F) - 'A' + 10)
+static guint8 *
+hwaddr_aton (const char *asc, guint8 *buffer, gsize buffer_length, gsize *out_len)
+{
+	const char *in = asc;
+	guint8 *out = buffer;
+	guint8 delimiter = '\0';
+
+	nm_assert (asc);
+	nm_assert (buffer);
+	nm_assert (buffer_length);
+	nm_assert (out_len);
+
+	while (TRUE) {
+		const guint8 d1 = in[0];
+		guint8 d2;
+
+		if (!g_ascii_isxdigit (d1))
+			return NULL;
+
+#define HEXVAL(c) ((c) <= '9' ? (c) - '0' : ((c) & 0x4F) - ('A' - 10))
+
+		/* If there's no leading zero (ie "aa:b:cc") then fake it */
+		d2 = in[1];
+		if (d2 && g_ascii_isxdigit (d2)) {
+			*out++ = (HEXVAL (d1) << 4) + HEXVAL (d2);
+			d2 = in[2];
+			in += 3;
+		} else {
+			/* Fake leading zero */
+			*out++ = HEXVAL (d1);
+			in += 2;
+		}
+
+		if (!d2)
+			break;
+		if (--buffer_length == 0)
+			return NULL;
+
+		if (d2 != delimiter) {
+			if (   delimiter == '\0'
+			    && (d2 == ':' || d2 == '-'))
+				delimiter = d2;
+			else
+				return NULL;
+		}
+	}
+
+	*out_len = out - buffer;
+	return buffer;
+}
 
 /**
  * nm_utils_hwaddr_atoba:
@@ -3022,18 +3069,52 @@ GByteArray *
 nm_utils_hwaddr_atoba (const char *asc, gsize length)
 {
 	GByteArray *ba;
+	gsize l;
 
-	g_return_val_if_fail (asc != NULL, NULL);
+	g_return_val_if_fail (asc, NULL);
 	g_return_val_if_fail (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX, NULL);
 
 	ba = g_byte_array_sized_new (length);
 	g_byte_array_set_size (ba, length);
-	if (!nm_utils_hwaddr_aton (asc, ba->data, length)) {
-		g_byte_array_unref (ba);
-		return NULL;
-	}
+	if (!hwaddr_aton (asc, ba->data, length, &l))
+		goto fail;
+	if (length != l)
+		goto fail;
 
 	return ba;
+fail:
+	g_byte_array_unref (ba);
+	return NULL;
+}
+
+/**
+ * _nm_utils_hwaddr_aton:
+ * @asc: the ASCII representation of a hardware address
+ * @buffer: buffer to store the result into. Must have
+ *   at least a size of @buffer_length.
+ * @buffer_length: the length of the input buffer @buffer.
+ *   The result must fit into that buffer, otherwise
+ *   the function fails and returns %NULL.
+ * @out_length: the output length in case of success.
+ *
+ * Parses @asc and converts it to binary form in @buffer.
+ * Bytes in @asc can be sepatared by colons (:), or hyphens (-), but not mixed.
+ *
+ * It is like nm_utils_hwaddr_aton(), but contrary to that it
+ * can parse addresses of any length. That is, you don't need
+ * to know the length before-hand.
+ *
+ * Return value: @buffer, or %NULL if @asc couldn't be parsed.
+ */
+guint8 *
+_nm_utils_hwaddr_aton (const char *asc, gpointer buffer, gsize buffer_length, gsize *out_length)
+{
+	g_return_val_if_fail (asc, NULL);
+	g_return_val_if_fail (buffer, NULL);
+	g_return_val_if_fail (buffer_length > 0, NULL);
+	g_return_val_if_fail (out_length, NULL);
+
+	return hwaddr_aton (asc, buffer, buffer_length, out_length);
 }
 
 /**
@@ -3052,72 +3133,55 @@ nm_utils_hwaddr_atoba (const char *asc, gsize length)
 guint8 *
 nm_utils_hwaddr_aton (const char *asc, gpointer buffer, gsize length)
 {
-	const char *in = asc;
-	guint8 *out = (guint8 *)buffer;
-	char delimiter = '\0';
+	gsize l;
 
-	g_return_val_if_fail (asc != NULL, NULL);
-	g_return_val_if_fail (buffer != NULL, NULL);
+	g_return_val_if_fail (asc, NULL);
+	g_return_val_if_fail (buffer, NULL);
 	g_return_val_if_fail (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX, NULL);
 
-	while (length && *in) {
-		guint8 d1 = in[0], d2 = in[1];
-
-		if (!g_ascii_isxdigit (d1))
-			return NULL;
-
-		/* If there's no leading zero (ie "aa:b:cc") then fake it */
-		if (d2 && g_ascii_isxdigit (d2)) {
-			*out++ = (HEXVAL (d1) << 4) + HEXVAL (d2);
-			in += 2;
-		} else {
-			/* Fake leading zero */
-			*out++ = (HEXVAL ('0') << 4) + HEXVAL (d1);
-			in += 1;
-		}
-
-		length--;
-		if (*in) {
-			if (delimiter == '\0') {
-				if (*in == ':' || *in == '-')
-					delimiter = *in;
-				else
-					return NULL;
-			} else {
-				if (*in != delimiter)
-					return NULL;
-			}
-			in++;
-		}
-	}
-
-	if (length == 0 && !*in)
-		return buffer;
-	else
+	if (!hwaddr_aton (asc, buffer, length, &l))
+		return NULL;
+	if (length != l)
 		return NULL;
+	return buffer;
 }
 
-static char *
-_bin2str (gconstpointer addr, gsize length, gboolean upper_case)
+static void
+_bin2str_buf (gconstpointer addr, gsize length, gboolean upper_case, char *out)
 {
 	const guint8 *in = addr;
-	char *out, *result;
 	const char *LOOKUP = upper_case ? "0123456789ABCDEF" : "0123456789abcdef";
 
-	g_return_val_if_fail (addr != NULL, g_strdup (""));
-	g_return_val_if_fail (length > 0, g_strdup (""));
+	nm_assert (addr);
+	nm_assert (out);
+	nm_assert (length > 0);
 
-	result = out = g_malloc (length * 3);
-	while (length--) {
-		guint8 v = *in++;
+	/* @out must contain at least @length*3 bytes */
+
+	for (;;) {
+		const guint8 v = *in++;
 
 		*out++ = LOOKUP[v >> 4];
 		*out++ = LOOKUP[v & 0x0F];
-		if (length)
-			*out++ = ':';
+		length--;
+		if (!length)
+			break;
+		*out++ = ':';
 	}
 
 	*out = 0;
+}
+
+static char *
+_bin2str (gconstpointer addr, gsize length, gboolean upper_case)
+{
+	char *result;
+
+	nm_assert (addr);
+	nm_assert (length > 0);
+
+	result = g_malloc (length * 3);
+	_bin2str_buf (addr, length, upper_case, result);
 	return result;
 }
 
@@ -3133,9 +3197,25 @@ _bin2str (gconstpointer addr, gsize length, gboolean upper_case)
 char *
 nm_utils_hwaddr_ntoa (gconstpointer addr, gsize length)
 {
+	g_return_val_if_fail (addr, g_strdup (""));
+	g_return_val_if_fail (length > 0, g_strdup (""));
+
 	return _bin2str (addr, length, TRUE);
 }
 
+const char *
+nm_utils_hwaddr_ntoa_buf (gconstpointer addr, gsize addr_len, gboolean upper_case, char *buf, gsize buf_len)
+{
+	g_return_val_if_fail (addr, NULL);
+	g_return_val_if_fail (addr_len > 0, NULL);
+	g_return_val_if_fail (buf, NULL);
+	if (buf_len < addr_len * 3)
+		g_return_val_if_reached (NULL);
+
+	_bin2str_buf (addr, addr_len, TRUE, buf);
+	return buf;
+}
+
 /**
  * _nm_utils_bin2str:
  * @addr: (type guint8) (array length=length): a binary hardware address
@@ -3149,49 +3229,10 @@ nm_utils_hwaddr_ntoa (gconstpointer addr, gsize length)
 char *
 _nm_utils_bin2str (gconstpointer addr, gsize length, gboolean upper_case)
 {
-	return _bin2str (addr, length, upper_case);
-}
-
-static int
-hwaddr_binary_len (const char *asc)
-{
-	int octets = 1;
-
-	if (!*asc)
-		return 0;
-
-	for (; *asc; asc++) {
-		if (*asc == ':' || *asc == '-')
-			octets++;
-	}
-	return octets;
-}
-
-/**
- * _nm_utils_hwaddr_length:
- * @asc: the ASCII representation of the hardware address
- *
- * Validates that @asc is a valid representation of a hardware
- * address up to (including) %NM_UTILS_HWADDR_LEN_MAX bytes.
- *
- * Returns: binary length of the hardware address @asc or
- *   0 on error.
- */
-guint
-_nm_utils_hwaddr_length (const char *asc)
-{
-	int l;
-
-	if (!asc)
-		return 0;
-
-	l = hwaddr_binary_len (asc);
-	if (l <= 0 || l > NM_UTILS_HWADDR_LEN_MAX)
-		return 0;
+	g_return_val_if_fail (addr, g_strdup (""));
+	g_return_val_if_fail (length > 0, g_strdup (""));
 
-	if (!nm_utils_hwaddr_valid (asc, l))
-		return 0;
-	return l;
+	return _bin2str (addr, length, upper_case);
 }
 
 /**
@@ -3210,17 +3251,18 @@ gboolean
 nm_utils_hwaddr_valid (const char *asc, gssize length)
 {
 	guint8 buf[NM_UTILS_HWADDR_LEN_MAX];
+	gsize l;
 
 	g_return_val_if_fail (asc != NULL, FALSE);
-	g_return_val_if_fail (length == -1 || (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX), FALSE);
 
-	if (length == -1) {
-		length = hwaddr_binary_len (asc);
-		if (length == 0 || length > NM_UTILS_HWADDR_LEN_MAX)
+	if (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX) {
+		if (!hwaddr_aton (asc, buf, length, &l))
 			return FALSE;
-	}
-
-	return nm_utils_hwaddr_aton (asc, buf, length) != NULL;
+		return length == l;
+	} else if (length == -1) {
+		return !!hwaddr_aton (asc, buf, sizeof (buf), &l);
+	} else
+		g_return_val_if_reached (FALSE);
 }
 
 /**
@@ -3240,20 +3282,23 @@ char *
 nm_utils_hwaddr_canonical (const char *asc, gssize length)
 {
 	guint8 buf[NM_UTILS_HWADDR_LEN_MAX];
+	gsize l;
 
-	g_return_val_if_fail (asc != NULL, NULL);
+	g_return_val_if_fail (asc, NULL);
 	g_return_val_if_fail (length == -1 || (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX), NULL);
 
-	if (length == -1) {
-		length = hwaddr_binary_len (asc);
-		if (length == 0 || length > NM_UTILS_HWADDR_LEN_MAX)
+	if (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX) {
+		if (!hwaddr_aton (asc, buf, length, &l))
 			return NULL;
-	}
-
-	if (nm_utils_hwaddr_aton (asc, buf, length) == NULL)
-		return NULL;
+		if (l != length)
+			return NULL;
+	} else if (length == -1) {
+		if (!hwaddr_aton (asc, buf, NM_UTILS_HWADDR_LEN_MAX, &l))
+			return NULL;
+	} else
+		g_return_val_if_reached (NULL);
 
-	return nm_utils_hwaddr_ntoa (buf, length);
+	return nm_utils_hwaddr_ntoa (buf, l);
 }
 
 /* This is used to possibly canonicalize values passed to MAC address property
@@ -3317,17 +3362,17 @@ nm_utils_hwaddr_matches (gconstpointer hwaddr1,
                          gssize        hwaddr2_len)
 {
 	guint8 buf1[NM_UTILS_HWADDR_LEN_MAX], buf2[NM_UTILS_HWADDR_LEN_MAX];
+	gsize l;
 
 	if (hwaddr1_len == -1) {
 		g_return_val_if_fail (hwaddr1 != NULL, FALSE);
 
-		hwaddr1_len = hwaddr_binary_len (hwaddr1);
-		if (hwaddr1_len == 0 || hwaddr1_len > NM_UTILS_HWADDR_LEN_MAX)
-			return FALSE;
-		if (!nm_utils_hwaddr_aton (hwaddr1, buf1, hwaddr1_len))
+		if (!hwaddr_aton (hwaddr1, buf1, sizeof (buf1), &l)) {
+			g_return_val_if_fail ((hwaddr2_len == -1 && hwaddr2) || (hwaddr2_len > 0 && hwaddr2_len <= NM_UTILS_HWADDR_LEN_MAX), FALSE);
 			return FALSE;
-
+		}
 		hwaddr1 = buf1;
+		hwaddr1_len = l;
 	} else {
 		g_return_val_if_fail (hwaddr1_len > 0 && hwaddr1_len <= NM_UTILS_HWADDR_LEN_MAX, FALSE);
 
@@ -3340,23 +3385,24 @@ nm_utils_hwaddr_matches (gconstpointer hwaddr1,
 	if (hwaddr2_len == -1) {
 		g_return_val_if_fail (hwaddr2 != NULL, FALSE);
 
-		if (!nm_utils_hwaddr_aton (hwaddr2, buf2, hwaddr1_len))
+		if (!hwaddr_aton (hwaddr2, buf2, sizeof (buf2), &l))
+			return FALSE;
+		if (l != hwaddr1_len)
 			return FALSE;
-
 		hwaddr2 = buf2;
 		hwaddr2_len = hwaddr1_len;
 	} else {
 		g_return_val_if_fail (hwaddr2_len > 0 && hwaddr2_len <= NM_UTILS_HWADDR_LEN_MAX, FALSE);
 
+		if (hwaddr2_len != hwaddr1_len)
+			return FALSE;
+
 		if (!hwaddr2) {
 			memset (buf2, 0, hwaddr2_len);
 			hwaddr2 = buf2;
 		}
 	}
 
-	if (hwaddr1_len != hwaddr2_len)
-		return FALSE;
-
 	if (hwaddr1_len == INFINIBAND_ALEN) {
 		hwaddr1 = (guint8 *)hwaddr1 + INFINIBAND_ALEN - 8;
 		hwaddr2 = (guint8 *)hwaddr2 + INFINIBAND_ALEN - 8;
@@ -3372,16 +3418,11 @@ static GVariant *
 _nm_utils_hwaddr_to_dbus_impl (const char *str)
 {
 	guint8 buf[NM_UTILS_HWADDR_LEN_MAX];
-	int len;
+	gsize len;
 
 	if (!str)
 		return NULL;
-
-	len = _nm_utils_hwaddr_length (str);
-	if (len == 0)
-		return NULL;
-
-	if (!nm_utils_hwaddr_aton (str, buf, len))
+	if (!hwaddr_aton (str, buf, sizeof (buf), &len))
 		return NULL;
 
 	return g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, buf, len, 1);
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 60c60f62e7..d7868f6502 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -11806,25 +11806,25 @@ nm_device_hw_addr_is_explict (NMDevice *self)
 }
 
 static gboolean
-_hw_addr_matches (NMDevice *self, const char *addr)
+_hw_addr_matches (NMDevice *self, const guint8 *addr, gsize addr_len)
 {
 	const char *cur_addr;
 
 	cur_addr = nm_device_get_hw_address (self);
-	return cur_addr && nm_utils_hwaddr_matches (cur_addr, -1, addr, -1);
+	return cur_addr && nm_utils_hwaddr_matches (addr, addr_len, cur_addr, -1);
 }
 
 static gboolean
 _hw_addr_set (NMDevice *self,
-              const char *addr,
-              const char *operation,
-              const char *detail)
+              const char *const addr,
+              const char *const operation,
+              const char *const detail)
 {
 	NMDevicePrivate *priv;
 	gboolean success = FALSE;
 	NMPlatformError plerr;
 	guint8 addr_bytes[NM_UTILS_HWADDR_LEN_MAX];
-	guint hw_addr_len;
+	gsize addr_len;
 	gboolean was_up;
 
 	nm_assert (NM_IS_DEVICE (self));
@@ -11833,18 +11833,20 @@ _hw_addr_set (NMDevice *self,
 
 	priv = NM_DEVICE_GET_PRIVATE (self);
 
+	if (!_nm_utils_hwaddr_aton (addr, addr_bytes, sizeof (addr_bytes), &addr_len))
+		g_return_val_if_reached (FALSE);
+
 	/* Do nothing if current MAC is same */
-	if (_hw_addr_matches (self, addr)) {
+	if (_hw_addr_matches (self, addr_bytes, addr_len)) {
 		_LOGT (LOGD_DEVICE, "set-hw-addr: no MAC address change needed (%s)", addr);
 		return TRUE;
 	}
 
-	hw_addr_len = priv->hw_addr_len;
-	if (!hw_addr_len)
-		hw_addr_len = _nm_utils_hwaddr_length (addr);
-	if (   !hw_addr_len
-	    || !nm_utils_hwaddr_aton (addr, addr_bytes, hw_addr_len))
-		g_return_val_if_reached (FALSE);
+	if (priv->hw_addr_len != addr_len) {
+		if (priv->hw_addr_len)
+			g_return_val_if_reached (FALSE);
+		priv->hw_addr_len = addr_len;
+	}
 
 	_LOGT (LOGD_DEVICE, "set-hw-addr: setting MAC address to '%s' (%s, %s)...", addr, operation, detail);
 
@@ -11854,12 +11856,12 @@ _hw_addr_set (NMDevice *self,
 		nm_device_take_down (self, FALSE);
 	}
 
-	plerr = nm_platform_link_set_address (NM_PLATFORM_GET, nm_device_get_ip_ifindex (self), addr_bytes, hw_addr_len);
+	plerr = nm_platform_link_set_address (NM_PLATFORM_GET, nm_device_get_ip_ifindex (self), addr_bytes, addr_len);
 	success = (plerr == NM_PLATFORM_ERROR_SUCCESS);
 	if (success) {
 		/* MAC address succesfully changed; update the current MAC to match */
 		nm_device_update_hw_address (self);
-		if (_hw_addr_matches (self, addr)) {
+		if (_hw_addr_matches (self, addr_bytes, addr_len)) {
 			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
 			       operation, addr, detail);
 		} else {
@@ -11889,7 +11891,7 @@ _hw_addr_set (NMDevice *self,
 					goto handle_fail;
 				if (!nm_device_update_hw_address (self))
 					goto handle_wait;
-				if (!_hw_addr_matches (self, addr))
+				if (!_hw_addr_matches (self, addr_bytes, addr_len))
 					goto handle_fail;
 
 				break;
@@ -12016,9 +12018,8 @@ nm_device_hw_addr_set_cloned (NMDevice *self, NMConnection *connection, gboolean
 		addr = hw_addr_generated;
 	} else {
 		/* this must be a valid address. Otherwise, we shouldn't come here. */
-		if (_nm_utils_hwaddr_length (addr) <= 0) {
+		if (!nm_utils_hwaddr_valid (addr, -1))
 			g_return_val_if_reached (FALSE);
-		}
 		priv->hw_addr_type = HW_ADDR_TYPE_EXPLICIT;
 	}
 
@@ -12217,13 +12218,16 @@ constructor (GType type,
 	}
 
 	if (priv->hw_addr_perm) {
-		priv->hw_addr_len = _nm_utils_hwaddr_length (priv->hw_addr_perm);
-		if (!priv->hw_addr_len) {
+		guint8 buf[NM_UTILS_HWADDR_LEN_MAX];
+		gsize l;
+
+		if (!_nm_utils_hwaddr_aton (priv->hw_addr_perm, buf, sizeof (buf), &l)) {
 			g_clear_pointer (&priv->hw_addr_perm, g_free);
 			g_return_val_if_reached (object);
 		}
 
-		priv->hw_addr = g_strdup (priv->hw_addr_perm);
+		priv->hw_addr_len = l;
+		priv->hw_addr = nm_utils_hwaddr_ntoa (buf, l);
 		_LOGT (LOGD_DEVICE, "hw-addr: has permanent hw-address '%s'", priv->hw_addr_perm);
 	}
 
diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c
index 984e3ec2ed..c48bf6cd45 100644
--- a/src/nm-core-utils.c
+++ b/src/nm-core-utils.c
@@ -1272,7 +1272,7 @@ nm_match_spec_hwaddr (const GSList *specs, const char *hwaddr)
 {
 	const GSList *iter;
 	NMMatchSpecMatchType match = NM_MATCH_SPEC_NO_MATCH;
-	guint hwaddr_len = 0;
+	gsize hwaddr_len = 0;
 	guint8 hwaddr_bin[NM_UTILS_HWADDR_LEN_MAX];
 
 	nm_assert (nm_utils_hwaddr_valid (hwaddr, -1));
@@ -1297,11 +1297,8 @@ nm_match_spec_hwaddr (const GSList *specs, const char *hwaddr)
 			continue;
 
 		if (G_UNLIKELY (hwaddr_len == 0)) {
-			hwaddr_len = _nm_utils_hwaddr_length (hwaddr);
-			if (!hwaddr_len)
+			if (!_nm_utils_hwaddr_aton (hwaddr, hwaddr_bin, sizeof (hwaddr_bin), &hwaddr_len))
 				g_return_val_if_reached (NM_MATCH_SPEC_NO_MATCH);
-			if (!nm_utils_hwaddr_aton (hwaddr, hwaddr_bin, hwaddr_len))
-				nm_assert_not_reached ();
 		}
 
 		if (nm_utils_hwaddr_matches (spec_str, -1, hwaddr_bin, hwaddr_len)) {