[PATCH iproute2 v2] ipaddress: strengthen check on 'label' input

[PATCH iproute2 v2] ipaddress: strengthen check on 'label' input

[Patrick Talbert]

As mentioned in the ip-address man page, an address label must
be equal to the device name or prefixed by the device name
followed by a colon. Currently the only check on this input is
to see if the device name appears at the beginning of the label
string.

This commit adds an additional check to ensure label == dev or
continues with a colon.

Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
---
 ip/ipaddress.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 00da14c..fce2008 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
 		return false;
 }
 
+static bool is_valid_label(const char *dev, const char *label)
+{
+	char alias[strlen(dev) + 1];
+
+	if (strlen(label) < strlen(dev))
+		return false;
+
+	strcpy(alias, dev);
+	strcat(alias, ":");
+	if (strncmp(label, dev, strlen(dev)) == 0 ||
+	    strncmp(label, alias, strlen(alias)) == 0)
+		return true;
+	else
+		return false;
+}
+
 static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 {
 	struct {
@@ -2174,8 +2190,9 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 		fprintf(stderr, "Not enough information: \"dev\" argument is required.\n");
 		return -1;
 	}
-	if (l && matches(d, l) != 0) {
-		fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d, l);
+	if (l && ! is_valid_label(d, l)) {
+		fprintf(stderr, "\"label\" (%s) must match \"dev\" (%s) or be prefixed by"
+			" \"dev\" with a colon.\n", l, d);
 		return -1;
 	}
 
-- 
1.8.3.1

Re: [PATCH iproute2 v2] ipaddress: strengthen check on 'label' input

[Stephen Hemminger]

On Tue, 29 May 2018 16:57:07 +0200
Patrick Talbert <ptalbert@redhat.com> wrote:

> As mentioned in the ip-address man page, an address label must
> be equal to the device name or prefixed by the device name
> followed by a colon. Currently the only check on this input is
> to see if the device name appears at the beginning of the label
> string.
> 
> This commit adds an additional check to ensure label == dev or
> continues with a colon.
> 
> Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>

Yes, this looks better but still have some feedback.

> ---
>  ip/ipaddress.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 00da14c..fce2008 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>  		return false;
>  }
>  
> +static bool is_valid_label(const char *dev, const char *label)
> +{
> +	char alias[strlen(dev) + 1];
> +
> +	if (strlen(label) < strlen(dev))
> +		return false;
> +
> +	strcpy(alias, dev);
> +	strcat(alias, ":");
> +	if (strncmp(label, dev, strlen(dev)) == 0 ||
> +	    strncmp(label, alias, strlen(alias)) == 0)
> +		return true;
> +	else
> +		return false;
> +}

This string copying and comparison still is much more overhead than it
needs to be. The following tests out to be equivalent with a single strncmp
and strlen.

Why not just:
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 00da14c6f97c..eac489e94fe4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2040,6 +2040,16 @@ static bool ipaddr_is_multicast(inet_prefix *a)
 		return false;
 }
 
+static bool is_valid_label(const char *label, const char *dev)
+{
+	size_t len = strlen(dev);
+
+	if (strncmp(label, dev, len) != 0)
+		return false;
+
+	return label[len] == '\0' || label[len] == ':';
+}
+
 

Doesn't matter much now, but code seems to get copied.

Re: [PATCH iproute2 v2] ipaddress: strengthen check on 'label' input

[Patrick Talbert]

On Fri, Jun 1, 2018 at 9:56 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Tue, 29 May 2018 16:57:07 +0200
> Patrick Talbert <ptalbert@redhat.com> wrote:
>
>> As mentioned in the ip-address man page, an address label must
>> be equal to the device name or prefixed by the device name
>> followed by a colon. Currently the only check on this input is
>> to see if the device name appears at the beginning of the label
>> string.
>>
>> This commit adds an additional check to ensure label == dev or
>> continues with a colon.
>>
>> Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
>> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
>
> Yes, this looks better but still have some feedback.
>
>> ---
>>  ip/ipaddress.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 00da14c..fce2008 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>>               return false;
>>  }
>>
>> +static bool is_valid_label(const char *dev, const char *label)
>> +{
>> +     char alias[strlen(dev) + 1];
>> +
>> +     if (strlen(label) < strlen(dev))
>> +             return false;
>> +
>> +     strcpy(alias, dev);
>> +     strcat(alias, ":");
>> +     if (strncmp(label, dev, strlen(dev)) == 0 ||
>> +         strncmp(label, alias, strlen(alias)) == 0)
>> +             return true;
>> +     else
>> +             return false;
>> +}
>
> This string copying and comparison still is much more overhead than it
> needs to be. The following tests out to be equivalent with a single strncmp
> and strlen.
>
> Why not just:
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 00da14c6f97c..eac489e94fe4 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -2040,6 +2040,16 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>                 return false;
>  }
>
> +static bool is_valid_label(const char *label, const char *dev)
> +{
> +       size_t len = strlen(dev);
> +
> +       if (strncmp(label, dev, len) != 0)
> +               return false;
> +
> +       return label[len] == '\0' || label[len] == ':';
> +}
> +

Woah. This is way better. v3 coming up....

Thank you for all of your help with this... and by help I mean writing
the patch.

>
>
> Doesn't matter much now, but code seems to get copied.