2010-01-17 20:59:53

by André Goddard Rosa

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

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(-)


2010-01-17 21:00:05

by André Goddard Rosa

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

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 <[email protected]>
cc: Joe Perches <[email protected]>
cc: Frederic Weisbecker <[email protected]>
cc: Andrew Morton <[email protected]>
---
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

2010-01-17 21:00:20

by André Goddard Rosa

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

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

Signed-off-by: André Goddard Rosa <[email protected]>
cc: Li Zefan <[email protected]>
cc: Alex Riesen <[email protected]>
cc: Andrew Morton <[email protected]>
---
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

2010-01-17 23:53:47

by Alex Riesen

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

On Sat, Jan 16, 2010 at 21:57, André Goddard Rosa
<[email protected]> 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.

2010-01-18 01:47:50

by Li Zefan

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

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 <[email protected]>
> cc: Li Zefan <[email protected]>
> cc: Alex Riesen <[email protected]>
> cc: Andrew Morton <[email protected]>
> ---
> 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))

2010-01-18 15:10:57

by André Goddard Rosa

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

On Sun, Jan 17, 2010 at 11:47 PM, Li Zefan <[email protected]> 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?

2010-01-23 00:28:31

by Andrew Morton

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

On Sat, 16 Jan 2010 18:57:00 -0200
Andr__ Goddard Rosa <[email protected]> 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 <[email protected]>
> cc: Joe Perches <[email protected]>
> cc: Frederic Weisbecker <[email protected]>
> cc: Andrew Morton <[email protected]>
> ---
> 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..

2010-01-23 01:47:45

by Linus Torvalds

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



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