[PATCH] arch: tile: gxio: mpipe.c: Cleaning up missing null-terminate in conjunction with strncpy

[PATCH] arch: tile: gxio: mpipe.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

Replacing strncpy with strlcpy to avoid strings that lacks null terminate.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/tile/gxio/mpipe.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index 5301a9f..4f1eefb 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -511,8 +511,7 @@ int gxio_mpipe_link_instance(const char *link_name)
 	if (!context)
 		return GXIO_ERR_NO_DEVICE;
 
-	strncpy(name.name, link_name, sizeof(name.name));
-	name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+	strlcpy(name.name, link_name, sizeof(name.name));
 
 	return gxio_mpipe_info_instance_aux(context, name);
 }
@@ -529,7 +528,7 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
 
 	rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
 	if (rv >= 0) {
-		strncpy(link_name, name.name, sizeof(name.name));
+		strlcpy(link_name, name.name, sizeof(name.name));
 		memcpy(link_mac, mac.mac, sizeof(mac.mac));
 	}
 
@@ -545,8 +544,7 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
 	_gxio_mpipe_link_name_t name;
 	int rv;
 
-	strncpy(name.name, link_name, sizeof(name.name));
-	name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+	strlcpy(name.name, link_name, sizeof(name.name));
 
 	rv = gxio_mpipe_link_open_aux(context, name, flags);
 	if (rv < 0)
-- 
1.7.10.4

Re: [PATCH] arch: tile: gxio: mpipe.c: Cleaning up missing null-terminate in conjunction with strncpy

[Chris Metcalf]

On 7/26/2014 10:03 AM, Rickard Strandqvist wrote:
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---

While using strlcpy() instead of strncpy() and an explicit NUL termination
is obviously an improvement, on reflection it doesn't feel like the best
solution here - and likely in many similar APIs.  A partial string argument
should not be passed on to other components; it's just an error by
definition and should be treated that way.  While we could check the
strlcpy() return value explicitly against the length, this is somewhat
awkward as it requires naming the length twice, and also means we end
up doing the copy pointlessly if we're about to return an error anyway.

The following change includes Yet Another String Copy function; this may
or may not be a fabulous idea, but at least it encapsulates the error
checking in a way that seems like it makes the call sites cleaner.


From: Chris Metcalf <cmetcalf@tilera.com>
Date: Tue, 5 Aug 2014 16:16:13 -0400
Subject: [PATCH] tile: avoid errors from truncating long strings in mpipe gxio

Using strncpy() will just silently truncate long strings; we should
instead return an appropriate error.  Using strlcpy() would suffer from
the same problem.  Instead, use strlen()+memcpy(), and add an
error-checking step to make sure the lengths are reasonable.

I called the convenience wrapper strscpy(), and a case could be made for
making it more generic (possibly with a better name), but that seems
outside the scope of this initial commit.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
  1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index 5301a9ffbae1..8b3c201dfdb7 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -29,6 +29,25 @@
  /* HACK: Avoid pointless "shadow" warnings. */
  #define link link_shadow

+/*
+ * Use this routine to avoid copying too-long strings.  Unlike strncpy
+ * or strlcpy, we don't enable programmers who don't check return codes;
+ * partially-copied strings can be problematic.  The routine returns
+ * the total number of bytes copied (including the trailing NUL) or
+ * zero if the buffer wasn't big enough.
+ */
+static size_t strscpy(char *dest, const char *src, size_t size)
+{
+       size_t ret = strlen(src) + 1;
+       if (ret > size) {
+               if (size)
+                       dest[0] = '\0';
+               return 0;
+       }
+       memcpy(dest, src, ret);
+       return ret;
+}
+
  int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index)
  {
         char file[32];
@@ -511,8 +530,8 @@ int gxio_mpipe_link_instance(const char *link_name)
         if (!context)
                 return GXIO_ERR_NO_DEVICE;

-       strncpy(name.name, link_name, sizeof(name.name));
-       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+               return GXIO_ERR_NO_DEVICE;

         return gxio_mpipe_info_instance_aux(context, name);
  }
@@ -529,7 +548,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)

         rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
         if (rv >= 0) {
-               strncpy(link_name, name.name, sizeof(name.name));
+               if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
+                       return GXIO_ERR_INVAL_MEMORY_SIZE;
                 memcpy(link_mac, mac.mac, sizeof(mac.mac));
         }

@@ -545,8 +565,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
         _gxio_mpipe_link_name_t name;
         int rv;

-       strncpy(name.name, link_name, sizeof(name.name));
-       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+               return GXIO_ERR_NO_DEVICE;

         rv = gxio_mpipe_link_open_aux(context, name, flags);
         if (rv < 0)
--
1.8.3.1

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch: tile: gxio: mpipe.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

2014-08-05 22:24 GMT+02:00 Chris Metcalf <cmetcalf@tilera.com>:
> On 7/26/2014 10:03 AM, Rickard Strandqvist wrote:
>>
>> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>>
>> Signed-off-by: Rickard Strandqvist
>> <rickard_strandqvist@spectrumdigital.se>
>> ---
>
>
> While using strlcpy() instead of strncpy() and an explicit NUL termination
> is obviously an improvement, on reflection it doesn't feel like the best
> solution here - and likely in many similar APIs.  A partial string argument
> should not be passed on to other components; it's just an error by
> definition and should be treated that way.  While we could check the
> strlcpy() return value explicitly against the length, this is somewhat
> awkward as it requires naming the length twice, and also means we end
> up doing the copy pointlessly if we're about to return an error anyway.
>
> The following change includes Yet Another String Copy function; this may
> or may not be a fabulous idea, but at least it encapsulates the error
> checking in a way that seems like it makes the call sites cleaner.
>
>
> From: Chris Metcalf <cmetcalf@tilera.com>
> Date: Tue, 5 Aug 2014 16:16:13 -0400
> Subject: [PATCH] tile: avoid errors from truncating long strings in mpipe
> gxio
>
> Using strncpy() will just silently truncate long strings; we should
> instead return an appropriate error.  Using strlcpy() would suffer from
> the same problem.  Instead, use strlen()+memcpy(), and add an
> error-checking step to make sure the lengths are reasonable.
>
> I called the convenience wrapper strscpy(), and a case could be made for
> making it more generic (possibly with a better name), but that seems
> outside the scope of this initial commit.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
>  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
> index 5301a9ffbae1..8b3c201dfdb7 100644
> --- a/arch/tile/gxio/mpipe.c
> +++ b/arch/tile/gxio/mpipe.c
> @@ -29,6 +29,25 @@
>  /* HACK: Avoid pointless "shadow" warnings. */
>  #define link link_shadow
>
> +/*
> + * Use this routine to avoid copying too-long strings.  Unlike strncpy
> + * or strlcpy, we don't enable programmers who don't check return codes;
> + * partially-copied strings can be problematic.  The routine returns
> + * the total number of bytes copied (including the trailing NUL) or
> + * zero if the buffer wasn't big enough.
> + */
> +static size_t strscpy(char *dest, const char *src, size_t size)
> +{
> +       size_t ret = strlen(src) + 1;
> +       if (ret > size) {
> +               if (size)
> +                       dest[0] = '\0';
> +               return 0;
> +       }
> +       memcpy(dest, src, ret);
> +       return ret;
> +}
> +
>  int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int
> mpipe_index)
>  {
>         char file[32];
> @@ -511,8 +530,8 @@ int gxio_mpipe_link_instance(const char *link_name)
>
>         if (!context)
>                 return GXIO_ERR_NO_DEVICE;
>
> -       strncpy(name.name, link_name, sizeof(name.name));
> -       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
> +       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
> +               return GXIO_ERR_NO_DEVICE;
>
>         return gxio_mpipe_info_instance_aux(context, name);
>  }
> @@ -529,7 +548,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char
> *link_name, uint8_t *link_mac)
>
>
>         rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
>         if (rv >= 0) {
> -               strncpy(link_name, name.name, sizeof(name.name));
> +               if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
> +                       return GXIO_ERR_INVAL_MEMORY_SIZE;
>                 memcpy(link_mac, mac.mac, sizeof(mac.mac));
>         }
>
> @@ -545,8 +565,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
>
>         _gxio_mpipe_link_name_t name;
>         int rv;
>
> -       strncpy(name.name, link_name, sizeof(name.name));
> -       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
> +       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
> +               return GXIO_ERR_NO_DEVICE;
>
>
>         rv = gxio_mpipe_link_open_aux(context, name, flags);
>         if (rv < 0)
> --
> 1.8.3.1
>
> --
> Chris Metcalf, Tilera Corp.
> http://www.tilera.com
>


Hi Chris!

Very good that something takes hold of this problem!

This has recently been discussed, where even Linus did a little post:

https://plus.google.com/111049168280159033135/posts/1amLbuhWbh5

This further clarifies the problem that really all of these functions
have same kind of problem. What Linus says about strlcpy I had not
heard before, but it's actually unusually unnecessary and stupid to
have that "feature".
Seeing that your strscpy() would have the same problem, change to
using strnlen() perhaps?


You want to have the more of a control, noaction on a error.
I would otherwise like to have an alternative to strncpy that something like:

char *strnzcpy(char *dest, const char *src, size_t count)
{
  if(0 == count)
    return dest;
  strncpy(dest, src, --count);
  dest[count] = '\0';
  return dest;
}

... But if we really going to rewrite it we should do it properly so
you can  get the equivalent to strlen as a return.
And then return = count than the string was too long. Simply return
the char *dest is so unnecessary :-)


Kind regards
Rickard Strandqvist

[PATCH v2] tile: avoid errors from truncating long strings in mpipe gxio

[Chris Metcalf]

Using strncpy() will just silently truncate long strings; we should
instead return an appropriate error.  Using strlcpy() would suffer from
the same problem.  Instead, use strnlen()+memcpy(), and add an
error-checking step to make sure the lengths are reasonable.

I called the convenience wrapper strscpy(), and a case could be made for
making it more generic (possibly with a better name), but that seems
outside the scope of this initial commit.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
v2: use strnlen instead of strlen

 arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index 5301a9ffbae1..27a56be8d583 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -29,6 +29,25 @@
 /* HACK: Avoid pointless "shadow" warnings. */
 #define link link_shadow
 
+/*
+ * Use this routine to avoid copying too-long strings.  Unlike strncpy
+ * or strlcpy, we don't enable programmers who don't check return codes;
+ * partially-copied strings can be problematic.  The routine returns
+ * the total number of bytes copied (including the trailing NUL) or
+ * zero if the buffer wasn't big enough.
+ */
+static size_t strscpy(char *dest, const char *src, size_t size)
+{
+	size_t ret = strnlen(src, size) + 1;
+	if (ret > size) {
+		if (size)
+			dest[0] = '\0';
+		return 0;
+	}
+	memcpy(dest, src, ret);
+	return ret;
+}
+
 int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index)
 {
 	char file[32];
@@ -511,8 +530,8 @@ int gxio_mpipe_link_instance(const char *link_name)
 	if (!context)
 		return GXIO_ERR_NO_DEVICE;
 
-	strncpy(name.name, link_name, sizeof(name.name));
-	name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+		return GXIO_ERR_NO_DEVICE;
 
 	return gxio_mpipe_info_instance_aux(context, name);
 }
@@ -529,7 +548,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
 
 	rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
 	if (rv >= 0) {
-		strncpy(link_name, name.name, sizeof(name.name));
+		if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
+			return GXIO_ERR_INVAL_MEMORY_SIZE;
 		memcpy(link_mac, mac.mac, sizeof(mac.mac));
 	}
 
@@ -545,8 +565,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
 	_gxio_mpipe_link_name_t name;
 	int rv;
 
-	strncpy(name.name, link_name, sizeof(name.name));
-	name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+		return GXIO_ERR_NO_DEVICE;
 
 	rv = gxio_mpipe_link_open_aux(context, name, flags);
 	if (rv < 0)
-- 
1.8.3.1

Re: [PATCH v2] tile: avoid errors from truncating long strings in mpipe gxio

[Rickard Strandqvist]

2014-08-06 20:16 GMT+02:00 Chris Metcalf <cmetcalf@tilera.com>:
> Using strncpy() will just silently truncate long strings; we should
> instead return an appropriate error.  Using strlcpy() would suffer from
> the same problem.  Instead, use strnlen()+memcpy(), and add an
> error-checking step to make sure the lengths are reasonable.
>
> I called the convenience wrapper strscpy(), and a case could be made for
> making it more generic (possibly with a better name), but that seems
> outside the scope of this initial commit.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
> v2: use strnlen instead of strlen
>
>  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
> index 5301a9ffbae1..27a56be8d583 100644
> --- a/arch/tile/gxio/mpipe.c
> +++ b/arch/tile/gxio/mpipe.c
> @@ -29,6 +29,25 @@
>  /* HACK: Avoid pointless "shadow" warnings. */
>  #define link link_shadow
>
> +/*
> + * Use this routine to avoid copying too-long strings.  Unlike strncpy
> + * or strlcpy, we don't enable programmers who don't check return codes;
> + * partially-copied strings can be problematic.  The routine returns
> + * the total number of bytes copied (including the trailing NUL) or
> + * zero if the buffer wasn't big enough.
> + */
> +static size_t strscpy(char *dest, const char *src, size_t size)
> +{
> +       size_t ret = strnlen(src, size) + 1;
> +       if (ret > size) {
> +               if (size)
> +                       dest[0] = '\0';
> +               return 0;
> +       }
> +       memcpy(dest, src, ret);
> +       return ret;
> +}
> +
>  int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index)
>  {
>         char file[32];
> @@ -511,8 +530,8 @@ int gxio_mpipe_link_instance(const char *link_name)
>         if (!context)
>                 return GXIO_ERR_NO_DEVICE;
>
> -       strncpy(name.name, link_name, sizeof(name.name));
> -       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
> +       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
> +               return GXIO_ERR_NO_DEVICE;
>
>         return gxio_mpipe_info_instance_aux(context, name);
>  }
> @@ -529,7 +548,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
>
>         rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
>         if (rv >= 0) {
> -               strncpy(link_name, name.name, sizeof(name.name));
> +               if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
> +                       return GXIO_ERR_INVAL_MEMORY_SIZE;
>                 memcpy(link_mac, mac.mac, sizeof(mac.mac));
>         }
>
> @@ -545,8 +565,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
>         _gxio_mpipe_link_name_t name;
>         int rv;
>
> -       strncpy(name.name, link_name, sizeof(name.name));
> -       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
> +       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
> +               return GXIO_ERR_NO_DEVICE;
>
>         rv = gxio_mpipe_link_open_aux(context, name, flags);
>         if (rv < 0)
> --
> 1.8.3.1
>


Looks good to me, and I did some tests to of the strncpy funktion.

Reviewed-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>

Kind regards
Rickard Strandqvist

Re: [PATCH v2] tile: avoid errors from truncating long strings in mpipe gxio

[Randy Dunlap]

On 08/06/14 11:16, Chris Metcalf wrote:
> Using strncpy() will just silently truncate long strings; we should
> instead return an appropriate error.  Using strlcpy() would suffer from
> the same problem.  Instead, use strnlen()+memcpy(), and add an
> error-checking step to make sure the lengths are reasonable.
> 
> I called the convenience wrapper strscpy(), and a case could be made for
> making it more generic (possibly with a better name), but that seems
> outside the scope of this initial commit.

Well, having looked at the function before I read this comment, my first
thought was that it should be added to lib/string.c for general
availability.

> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
> v2: use strnlen instead of strlen
> 
>  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
> index 5301a9ffbae1..27a56be8d583 100644
> --- a/arch/tile/gxio/mpipe.c
> +++ b/arch/tile/gxio/mpipe.c
> @@ -29,6 +29,25 @@
>  /* HACK: Avoid pointless "shadow" warnings. */
>  #define link link_shadow
>  
> +/*
> + * Use this routine to avoid copying too-long strings.  Unlike strncpy
> + * or strlcpy, we don't enable programmers who don't check return codes;
> + * partially-copied strings can be problematic.  The routine returns
> + * the total number of bytes copied (including the trailing NUL) or
> + * zero if the buffer wasn't big enough.
> + */
> +static size_t strscpy(char *dest, const char *src, size_t size)
> +{
> +	size_t ret = strnlen(src, size) + 1;
> +	if (ret > size) {
> +		if (size)
> +			dest[0] = '\0';
> +		return 0;
> +	}
> +	memcpy(dest, src, ret);
> +	return ret;
> +}

-- 
~Randy

new generic strscpy API? (was Re: [PATCH v2] tile: avoid errors from truncating long strings in mpipe gxio)

[Chris Metcalf]

On 8/7/2014 2:43 AM, Randy Dunlap wrote:
> On 08/06/14 11:16, Chris Metcalf wrote:
>> Using strncpy() will just silently truncate long strings; we should
>> instead return an appropriate error.  Using strlcpy() would suffer from
>> the same problem.  Instead, use strnlen()+memcpy(), and add an
>> error-checking step to make sure the lengths are reasonable.
>>
>> I called the convenience wrapper strscpy(), and a case could be made for
>> making it more generic (possibly with a better name), but that seems
>> outside the scope of this initial commit.
> Well, having looked at the function before I read this comment, my first
> thought was that it should be added to lib/string.c for general
> availability.

I'm happy to do that, but it probably shouldn't go through the linux-tile
tree in that case, since I'd be touching platform-independent code.
If someone wants to volunteer to push a new lib/strscpy.c change to Linus
(presumably including the arch/tile caller) I'm happy to redo this commit
in that form.

My guess is that we also haven't hit the mandatory minimum of
bike-shedding around function name and precise semantics yet, anyway :-)

I will hold off on pushing this change until a bit later in the merge
window to see if anyone wants to jump in.

>> diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
>> index 5301a9ffbae1..27a56be8d583 100644
>> --- a/arch/tile/gxio/mpipe.c
>> +++ b/arch/tile/gxio/mpipe.c
>> @@ -29,6 +29,25 @@
>>   /* HACK: Avoid pointless "shadow" warnings. */
>>   #define link link_shadow
>>   
>> +/*
>> + * Use this routine to avoid copying too-long strings.  Unlike strncpy
>> + * or strlcpy, we don't enable programmers who don't check return codes;
>> + * partially-copied strings can be problematic.  The routine returns
>> + * the total number of bytes copied (including the trailing NUL) or
>> + * zero if the buffer wasn't big enough.
>> + */
>> +static size_t strscpy(char *dest, const char *src, size_t size)
>> +{
>> +	size_t ret = strnlen(src, size) + 1;
>> +	if (ret > size) {
>> +		if (size)
>> +			dest[0] = '\0';
>> +		return 0;
>> +	}
>> +	memcpy(dest, src, ret);
>> +	return ret;
>> +}

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH v2] tile: avoid errors from truncating long strings in mpipe gxio

[Rickard Strandqvist]

2014-08-07 8:43 GMT+02:00 Randy Dunlap <rdunlap@infradead.org>:
> On 08/06/14 11:16, Chris Metcalf wrote:
>> Using strncpy() will just silently truncate long strings; we should
>> instead return an appropriate error.  Using strlcpy() would suffer from
>> the same problem.  Instead, use strnlen()+memcpy(), and add an
>> error-checking step to make sure the lengths are reasonable.
>>
>> I called the convenience wrapper strscpy(), and a case could be made for
>> making it more generic (possibly with a better name), but that seems
>> outside the scope of this initial commit.
>
> Well, having looked at the function before I read this comment, my first
> thought was that it should be added to lib/string.c for general
> availability.
>
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
>> ---
>> v2: use strnlen instead of strlen
>>
>>  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
>> index 5301a9ffbae1..27a56be8d583 100644
>> --- a/arch/tile/gxio/mpipe.c
>> +++ b/arch/tile/gxio/mpipe.c
>> @@ -29,6 +29,25 @@
>>  /* HACK: Avoid pointless "shadow" warnings. */
>>  #define link link_shadow
>>
>> +/*
>> + * Use this routine to avoid copying too-long strings.  Unlike strncpy
>> + * or strlcpy, we don't enable programmers who don't check return codes;
>> + * partially-copied strings can be problematic.  The routine returns
>> + * the total number of bytes copied (including the trailing NUL) or
>> + * zero if the buffer wasn't big enough.
>> + */
>> +static size_t strscpy(char *dest, const char *src, size_t size)
>> +{
>> +     size_t ret = strnlen(src, size) + 1;
>> +     if (ret > size) {
>> +             if (size)
>> +                     dest[0] = '\0';
>> +             return 0;
>> +     }
>> +     memcpy(dest, src, ret);
>> +     return ret;
>> +}
>
> --
> ~Randy


Hi

Sounds like a great idea!

Do it Chris!  Randy?


Kind regards
Rickard Strandqvist

Re: [PATCH v2] tile: avoid errors from truncating long strings in mpipe gxio

[Randy Dunlap]

On 08/11/14 13:43, Rickard Strandqvist wrote:
> 2014-08-07 8:43 GMT+02:00 Randy Dunlap <rdunlap@infradead.org>:
>> On 08/06/14 11:16, Chris Metcalf wrote:
>>> Using strncpy() will just silently truncate long strings; we should
>>> instead return an appropriate error.  Using strlcpy() would suffer from
>>> the same problem.  Instead, use strnlen()+memcpy(), and add an
>>> error-checking step to make sure the lengths are reasonable.
>>>
>>> I called the convenience wrapper strscpy(), and a case could be made for
>>> making it more generic (possibly with a better name), but that seems
>>> outside the scope of this initial commit.
>>
>> Well, having looked at the function before I read this comment, my first
>> thought was that it should be added to lib/string.c for general
>> availability.
>>
>>>
>>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
>>> ---
>>> v2: use strnlen instead of strlen
>>>
>>>  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
>>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
>>> index 5301a9ffbae1..27a56be8d583 100644
>>> --- a/arch/tile/gxio/mpipe.c
>>> +++ b/arch/tile/gxio/mpipe.c
>>> @@ -29,6 +29,25 @@
>>>  /* HACK: Avoid pointless "shadow" warnings. */
>>>  #define link link_shadow
>>>
>>> +/*
>>> + * Use this routine to avoid copying too-long strings.  Unlike strncpy
>>> + * or strlcpy, we don't enable programmers who don't check return codes;
>>> + * partially-copied strings can be problematic.  The routine returns
>>> + * the total number of bytes copied (including the trailing NUL) or
>>> + * zero if the buffer wasn't big enough.
>>> + */
>>> +static size_t strscpy(char *dest, const char *src, size_t size)
>>> +{
>>> +     size_t ret = strnlen(src, size) + 1;
>>> +     if (ret > size) {
>>> +             if (size)
>>> +                     dest[0] = '\0';
>>> +             return 0;
>>> +     }
>>> +     memcpy(dest, src, ret);
>>> +     return ret;
>>> +}
>>
>> --
>> ~Randy
> 
> 
> Hi
> 
> Sounds like a great idea!
> 
> Do it Chris!  Randy?

Yeah, I don't have a better name for it, but I do think
that it's the right thing to do.

Thanks,

-- 
~Randy

[PATCH 1/2] Add strscpy() as a new string copy primitive

[Chris Metcalf]

Both strncpy and strlcpy suffer from the fact that they do
partial copies of strings into the destination when the target
buffer is too small.  This is frequently pointless since an
overflow of the target buffer may make the result invalid.

strncpy() makes it relatively hard to even detect the error
condition, and with strlcpy() you have to duplicate the buffer
size parameter to test to see if the result exceeds it.
By returning zero in the failure case, we both make testing
for it easy, and by simply not copying anything in that case,
we make it mandatory for callers to test the error code.

To catch lazy programmers who don't check, we also place a NUL at
the start of the destination buffer (if there is space) to
ensure that the result is an invalid string.

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Reviewed-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 include/linux/string.h |  3 +++
 lib/string.c           | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index d36977e029af..b9e9b2b5f6c6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -25,6 +25,9 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
 #ifndef __HAVE_ARCH_STRLCPY
 size_t strlcpy(char *, const char *, size_t);
 #endif
+#ifndef __HAVE_ARCH_STRSCPY
+size_t strscpy(char *, const char *, size_t);
+#endif
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif
diff --git a/lib/string.c b/lib/string.c
index 992bf30af759..bbd67b3343dc 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -160,6 +160,35 @@ size_t strlcpy(char *dest, const char *src, size_t size)
 EXPORT_SYMBOL(strlcpy);
 #endif
 
+#ifndef __HAVE_ARCH_STRSCPY
+/**
+ * strscpy - Copy a C-string into a sized buffer, but only if it fits
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @size: size of destination buffer
+ *
+ * Use this routine to avoid copying too-long strings.
+ * The routine returns the total number of bytes copied
+ * (including the trailing NUL) or zero if the buffer wasn't
+ * big enough.  To ensure that programmers pay attention
+ * to the return code, the destination has a single NUL
+ * written at the front (if size is non-zero) when the
+ * buffer is not big enough.
+ */
+size_t strscpy(char *dest, const char *src, size_t size)
+{
+	size_t len = strnlen(src, size) + 1;
+	if (len > size) {
+		if (size)
+			dest[0] = '\0';
+		return 0;
+	}
+	memcpy(dest, src, len);
+	return len;
+}
+EXPORT_SYMBOL(strscpy);
+#endif
+
 #ifndef __HAVE_ARCH_STRCAT
 /**
  * strcat - Append one %NUL-terminated string to another
-- 
1.8.3.1

[PATCH 2/2] tile gxio mpipe: use the new strscpy() primitive

[Chris Metcalf]

This makes it possible to easily test for overflow conditions
and return suitable errors to the callers.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/gxio/mpipe.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index 5301a9ffbae1..0f02e118d0e3 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -511,8 +511,8 @@ int gxio_mpipe_link_instance(const char *link_name)
 	if (!context)
 		return GXIO_ERR_NO_DEVICE;
 
-	strncpy(name.name, link_name, sizeof(name.name));
-	name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+		return GXIO_ERR_NO_DEVICE;
 
 	return gxio_mpipe_info_instance_aux(context, name);
 }
@@ -529,7 +529,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
 
 	rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
 	if (rv >= 0) {
-		strncpy(link_name, name.name, sizeof(name.name));
+		if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
+			return GXIO_ERR_INVAL_MEMORY_SIZE;
 		memcpy(link_mac, mac.mac, sizeof(mac.mac));
 	}
 
@@ -545,8 +546,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
 	_gxio_mpipe_link_name_t name;
 	int rv;
 
-	strncpy(name.name, link_name, sizeof(name.name));
-	name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+	if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+		return GXIO_ERR_NO_DEVICE;
 
 	rv = gxio_mpipe_link_open_aux(context, name, flags);
 	if (rv < 0)
-- 
1.8.3.1