[PATCH 0/2] Fix recently introduced strnstr() to not search past NUL

[PATCH 0/2] Fix recently introduced strnstr() to not search past NUL

[André Goddard Rosa]

Also simplify stricmp(), so that we do not increase lib/string.o code size.

André Goddard Rosa (2):
  string: simplify stricmp()
  string: teach strnstr() to not search past NUL-terminator

 lib/string.c |   38 ++++++++++++++++++--------------------
 1 files changed, 18 insertions(+), 20 deletions(-)

[PATCH 1/2] string: simplify stricmp()

[André Goddard Rosa]

Removes 32 bytes on core2 with gcc 4.4.1:
   text    data     bss     dec     hex filename
   3196       0       0    3196     c7c lib/string-BEFORE.o
   3164       0       0    3164     c5c lib/string-AFTER.o

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
cc: Joe Perches <joe@perches.com>
cc: Frederic Weisbecker <fweisbec@gmail.com>
cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/string.c |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index a1cdcfc..0f86245 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -36,25 +36,21 @@ int strnicmp(const char *s1, const char *s2, size_t len)
 	/* Yes, Virginia, it had better be unsigned */
 	unsigned char c1, c2;
 
-	c1 = c2 = 0;
-	if (len) {
-		do {
-			c1 = *s1;
-			c2 = *s2;
-			s1++;
-			s2++;
-			if (!c1)
-				break;
-			if (!c2)
-				break;
-			if (c1 == c2)
-				continue;
-			c1 = tolower(c1);
-			c2 = tolower(c2);
-			if (c1 != c2)
-				break;
-		} while (--len);
-	}
+	if (!len)
+		return 0;
+
+	do {
+		c1 = *s1++;
+		c2 = *s2++;
+		if (!c1 || !c2)
+			break;
+		if (c1 == c2)
+			continue;
+		c1 = tolower(c1);
+		c2 = tolower(c2);
+		if (c1 != c2)
+			break;
+	} while (--len);
 	return (int)c1 - (int)c2;
 }
 EXPORT_SYMBOL(strnicmp);
-- 
1.6.6.201.g56119

Re: [PATCH 1/2] string: simplify stricmp()

[Andrew Morton]

On Sat, 16 Jan 2010 18:57:00 -0200
Andr__ Goddard Rosa <andre.goddard@gmail.com> wrote:

> Removes 32 bytes on core2 with gcc 4.4.1:
>    text    data     bss     dec     hex filename
>    3196       0       0    3196     c7c lib/string-BEFORE.o
>    3164       0       0    3164     c5c lib/string-AFTER.o
> 
> Signed-off-by: Andr__ Goddard Rosa <andre.goddard@gmail.com>
> cc: Joe Perches <joe@perches.com>
> cc: Frederic Weisbecker <fweisbec@gmail.com>
> cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  lib/string.c |   34 +++++++++++++++-------------------
>  1 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/string.c b/lib/string.c
> index a1cdcfc..0f86245 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -36,25 +36,21 @@ int strnicmp(const char *s1, const char *s2, size_t len)
>  	/* Yes, Virginia, it had better be unsigned */
>  	unsigned char c1, c2;
>  
> -	c1 = c2 = 0;
> -	if (len) {
> -		do {
> -			c1 = *s1;
> -			c2 = *s2;
> -			s1++;
> -			s2++;
> -			if (!c1)
> -				break;
> -			if (!c2)
> -				break;
> -			if (c1 == c2)
> -				continue;
> -			c1 = tolower(c1);
> -			c2 = tolower(c2);
> -			if (c1 != c2)
> -				break;
> -		} while (--len);
> -	}
> +	if (!len)
> +		return 0;
> +
> +	do {
> +		c1 = *s1++;
> +		c2 = *s2++;
> +		if (!c1 || !c2)
> +			break;
> +		if (c1 == c2)
> +			continue;
> +		c1 = tolower(c1);
> +		c2 = tolower(c2);
> +		if (c1 != c2)
> +			break;
> +	} while (--len);
>  	return (int)c1 - (int)c2;
>  }
>  EXPORT_SYMBOL(strnicmp);

hm, that function seems a little broken.

If it reaches the end of s1 or s2 it will return (c1 - c2).  If however
it detects a difference due to other than end-of-string, it returns
(tolower(c1) - tolower(c2)).

IOW, perhaps it should be performing tolower() in the
I-reached-end-of-string case.

I wonder what strnicmp() is _supposed_ to return..

Re: [PATCH 1/2] string: simplify stricmp()

[Linus Torvalds]

On Fri, 22 Jan 2010, Andrew Morton wrote:
> 
> hm, that function seems a little broken.
> 
> If it reaches the end of s1 or s2 it will return (c1 - c2).  If however
> it detects a difference due to other than end-of-string, it returns
> (tolower(c1) - tolower(c2)).
> 
> IOW, perhaps it should be performing tolower() in the
> I-reached-end-of-string case.

No, it doesn't matter. It's like strcmp - it returns "positive, negative 
or zero" depending on whether the string compared bigger than, smaller 
than or equal.

So "(int)c1 - (int)c2" is just a way to do that. And if the string ends 
and c1 or c2 is NUL, there's no point in doing the "tolower()", because it 
doesn't matter what the other character is: the sign is all that matters 
(or, if they are equally long, and both c1 and c2 are zero, then 0).

> I wonder what strnicmp() is _supposed_ to return..

Same as strncmp.

We could make it always return -1/0/+1 (many libraries do that - then the 
return value is guaranteed to also fit in a 'char', not just an 'int'), 
but it really shouldn't matter for any correct caller.

		Linus

[PATCH 2/2] string: teach strnstr() to not search past NUL-terminator

[André Goddard Rosa]

As noticed by Alex Riesen at:
http://marc.info/?l=linux-kernel&m=126364040821071&w=2

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
cc: Li Zefan <lizf@cn.fujitsu.com>
cc: Alex Riesen <raa.lkml@gmail.com>
cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/string.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index 0f86245..94bad34 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -689,11 +689,13 @@ EXPORT_SYMBOL(strstr);
  */
 char *strnstr(const char *s1, const char *s2, size_t len)
 {
-	size_t l1 = len, l2;
+	size_t l1, l2;
 
 	l2 = strlen(s2);
 	if (!l2)
 		return (char *)s1;
+	l1 = strlen(s1);
+	l1 = (l1 <= len) ? l1 : len;
 	while (l1 >= l2) {
 		l1--;
 		if (!memcmp(s1, s2, l2))
-- 
1.6.6.201.g56119

Re: [PATCH 2/2] string: teach strnstr() to not search past NUL-terminator

[Alex Riesen]

On Sat, Jan 16, 2010 at 21:57, André Goddard Rosa
<andre.goddard@gmail.com> wrote:
> diff --git a/lib/string.c b/lib/string.c
> index 0f86245..94bad34 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -689,11 +689,13 @@ EXPORT_SYMBOL(strstr);
>  */
>  char *strnstr(const char *s1, const char *s2, size_t len)
>  {
> -       size_t l1 = len, l2;
> +       size_t l1, l2;
>
>        l2 = strlen(s2);
>        if (!l2)
>                return (char *)s1;
> +       l1 = strlen(s1);
> +       l1 = (l1 <= len) ? l1 : len;

if (l1 > len)
       l1 = len;

Less code and easier to read.

Re: [PATCH 2/2] string: teach strnstr() to not search past NUL-terminator

[Li Zefan]

André Goddard Rosa wrote:
> As noticed by Alex Riesen at:
> http://marc.info/?l=linux-kernel&m=126364040821071&w=2
> 

Please don't make this change, it's the user's responsibility
to make sure @len won't exceed @s1's length.

All the string functions of "n" version should work when
@s1 is not null-terminated, so don't call strlen() on @s1.

> Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
> cc: Li Zefan <lizf@cn.fujitsu.com>
> cc: Alex Riesen <raa.lkml@gmail.com>
> cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  lib/string.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/string.c b/lib/string.c
> index 0f86245..94bad34 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -689,11 +689,13 @@ EXPORT_SYMBOL(strstr);
>   */
>  char *strnstr(const char *s1, const char *s2, size_t len)
>  {
> -	size_t l1 = len, l2;
> +	size_t l1, l2;
>  
>  	l2 = strlen(s2);
>  	if (!l2)
>  		return (char *)s1;
> +	l1 = strlen(s1);
> +	l1 = (l1 <= len) ? l1 : len;
>  	while (l1 >= l2) {
>  		l1--;
>  		if (!memcmp(s1, s2, l2))

Re: [PATCH 2/2] string: teach strnstr() to not search past NUL-terminator

[André Goddard Rosa]

On Sun, Jan 17, 2010 at 11:47 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> André Goddard Rosa wrote:
>> As noticed by Alex Riesen at:
>> http://marc.info/?l=linux-kernel&m=126364040821071&w=2
>>
> Please don't make this change, it's the user's responsibility
> to make sure @len won't exceed @s1's length.
>
> All the string functions of "n" version should work when
> @s1 is not null-terminated, so don't call strlen() on @s1.

Fair enough, agreed.

Thank you,
André