[PATCH] x86,boot: standardize strcmp()

[PATCH] x86,boot: standardize strcmp()

[Arjun Sreedharan]

strcmp() is always expected to return 0 when args are
same, <0 when arg1 is lesser and >0 otherwise.
Previously strcmp("a","b") returned 1. Now it gives -1.

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 493f3fd..318b846 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;
-- 
1.8.1.msysgit.1

Re: [PATCH] x86,boot: standardize strcmp()

[Borislav Petkov]

On Mon, Mar 16, 2015 at 09:07:47PM +0530, Arjun Sreedharan wrote:
> strcmp() is always expected to return 0 when args are
> same, <0 when arg1 is lesser and >0 otherwise.
> Previously strcmp("a","b") returned 1. Now it gives -1.
> 
> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
> ---
>  arch/x86/boot/string.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> index 493f3fd..318b846 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
>  	int delta = 0;
>  
>  	while (*s1 || *s2) {
> -		delta = *s2 - *s1;
> +		delta = *s1 - *s2;
>  		if (delta)
>  			return delta;
>  		s1++;
> -- 

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86,boot: standardize strcmp()

[Ingo Molnar]

* Arjun Sreedharan <arjun024@gmail.com> wrote:

> strcmp() is always expected to return 0 when args are
> same, <0 when arg1 is lesser and >0 otherwise.
> Previously strcmp("a","b") returned 1. Now it gives -1.

I'd also add the following to the changelog:

Until now this bug never triggered, because all uses for strcmp() in 
the boot code tested for nonzero:

 triton:~/tip> git grep strcmp arch/x86/boot/
 arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
 arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
 arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
 arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))

should in the future strcmp() be used in a comparative way in the boot 
code, it might have led to (not so subtle) bugs.

Thanks,

	Ingo

Re: [PATCH] x86,boot: standardize strcmp()

[Arjun Sreedharan]

On 17 March 2015 at 13:16, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Arjun Sreedharan <arjun024@gmail.com> wrote:
>
>> strcmp() is always expected to return 0 when args are
>> same, <0 when arg1 is lesser and >0 otherwise.
>> Previously strcmp("a","b") returned 1. Now it gives -1.
>
> I'd also add the following to the changelog:
>
> Until now this bug never triggered, because all uses for strcmp() in
> the boot code tested for nonzero:

On a related note, IMO strcmp() should return {-1,0,1} since many
programmers just expect this behavior. just my opinion.

Arjun Sreedharan

>
>  triton:~/tip> git grep strcmp arch/x86/boot/
>  arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
>  arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
>  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
>  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))
>
> should in the future strcmp() be used in a comparative way in the boot
> code, it might have led to (not so subtle) bugs.
>
> Thanks,
>
>         Ingo

Re: [PATCH] x86,boot: standardize strcmp()

[Borislav Petkov]

On Tue, Mar 17, 2015 at 08:46:08AM +0100, Ingo Molnar wrote:
> I'd also add the following to the changelog:

Added:

---
From: Arjun Sreedharan <arjun024@gmail.com>
Date: Mon, 16 Mar 2015 21:07:47 +0530
Subject: [PATCH] x86/boot: Standardize strcmp()

strcmp() is always expected to return 0 when arguments are equal,
negative when its first argument @str1 is less than its second argument
@str2 and a positive value otherwise. Previously strcmp("a", "b")
returned 1. Now it gives -1, as it is supposed to.

Until now this bug never triggered, because all uses for strcmp() in the
boot code tested for nonzero:

  triton:~/tip> git grep strcmp arch/x86/boot/
  arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
  arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))

should in the future strcmp() be used in a comparative way in the boot
code, it might have led to (not so subtle) bugs.

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Link: http://lkml.kernel.org/r/1426520267-1803-1-git-send-email-arjun024@gmail.com
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 493f3fd9f139..318b8465d302 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;
-- 
2.3.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86,boot: standardize strcmp()

[Bernd Petrovitsch]

On Die, 2015-03-17 at 19:43 +0530, Arjun Sreedharan wrote:
[...]
> On a related note, IMO strcmp() should return {-1,0,1} since many
> programmers just expect this behavior. just my opinion.

-ENOPATCH.

MfG,
	Bernd
-- 
"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
    - Linus Torvalds

Re: [PATCH] x86,boot: standardize strcmp()

[Arjun Sreedharan]

On 18 March 2015 at 07:06, Bernd Petrovitsch <bernd@petrovitsch.priv.at> wrote:
> On Die, 2015-03-17 at 19:43 +0530, Arjun Sreedharan wrote:
> [...]
>> On a related note, IMO strcmp() should return {-1,0,1} since many
>> programmers just expect this behavior. just my opinion.
>
> -ENOPATCH.

Here's a patch

-- >8 --

Subject: [PATCH] arm,x86: limit strcmp() rc to {-1,0,1}

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
---
 arch/arm/boot/compressed/string.c | 2 +-
 arch/x86/boot/string.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/compressed/string.c
b/arch/arm/boot/compressed/string.c
index 36e53ef..e48df86 100644
--- a/arch/arm/boot/compressed/string.c
+++ b/arch/arm/boot/compressed/string.c
@@ -88,7 +88,7 @@ int strcmp(const char *cs, const char *ct)
  c2 = *ct++;
  res = c1 - c2;
  if (res)
- break;
+ return res < 0 ? -1 : 1;
  } while (c1);
  return res;
 }
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 318b846..6eb333e 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -32,7 +32,7 @@ int strcmp(const char *str1, const char *str2)
  while (*s1 || *s2) {
  delta = *s1 - *s2;
  if (delta)
- return delta;
+ return delta < 0 ? -1 : 1;
  s1++;
  s2++;
  }
-- 

>
> MfG,
>         Bernd
> --
> "I dislike type abstraction if it has no real reason. And saving
> on typing is not a good reason - if your typing speed is the main
> issue when you're coding, you're doing something seriously wrong."
>     - Linus Torvalds
>

[PATCH] x86,boot: standardize strcmp()

[Arjun Sreedharan]

Sorry for the previous messed up email.
Here's a patch.

-- >8 --

Subject: [PATCH] arm,x86: limit strcmp() rc to {-1,0,1}

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
---
 arch/arm/boot/compressed/string.c | 2 +-
 arch/x86/boot/string.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/compressed/string.c b/arch/arm/boot/compressed/string.c
index 36e53ef..e48df86 100644
--- a/arch/arm/boot/compressed/string.c
+++ b/arch/arm/boot/compressed/string.c
@@ -88,7 +88,7 @@ int strcmp(const char *cs, const char *ct)
 		c2 = *ct++;
 		res = c1 - c2;
 		if (res)
-			break;
+			return res < 0 ? -1 : 1;
 	} while (c1);
 	return res;
 }
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 318b846..6eb333e 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -32,7 +32,7 @@ int strcmp(const char *str1, const char *str2)
 	while (*s1 || *s2) {
 		delta = *s1 - *s2;
 		if (delta)
-			return delta;
+			return delta < 0 ? -1 : 1;
 		s1++;
 		s2++;
 	}
-- 

Re: [PATCH] x86,boot: standardize strcmp()

[H. Peter Anvin]

On 03/17/2015 07:13 AM, Arjun Sreedharan wrote:
> 
> On a related note, IMO strcmp() should return {-1,0,1} since many
> programmers just expect this behavior. just my opinion.
> 

I would challenge that assumption, *especially* in the context of kernel
programming.  Let's not waste time on that crap.

	-hpa

Re: [PATCH] x86,boot: standardize strcmp()

[Bernd Petrovitsch]

On Don, 2015-03-19 at 10:34 -0700, H. Peter Anvin wrote:
> On 03/17/2015 07:13 AM, Arjun Sreedharan wrote:
> > On a related note, IMO strcmp() should return {-1,0,1} since many
> > programmers just expect this behavior. just my opinion.

One doesn't change an API just for a claimed expection for an unprooved
number of cases.

> I would challenge that assumption, *especially* in the context of kernel
> programming.  Let's not waste time on that crap.

Even if the assumption is correct (which I'm not implying - quite the
opposite), than these programmers are not well educated enough and -
thus;-) - write buggy code. They also fail to strive for mot possible
robustness.

BTW POSIX' strcmp() description on
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strcmp.html
als states "> 0, == 0 or < 0" (and ISO-C seem to also see it that way).

Kind regards,
	Bernd
-- 
"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
    - Linus Torvalds

[tip:x86/boot] x86/boot: Standardize strcmp()

[tip-bot for Arjun Sreedharan]

Commit-ID:  1c1d046be692493d00a4831d4fbc266745008e09
Gitweb:     http://git.kernel.org/tip/1c1d046be692493d00a4831d4fbc266745008e09
Author:     Arjun Sreedharan <arjun024@gmail.com>
AuthorDate: Mon, 16 Mar 2015 21:07:47 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 23 Mar 2015 10:24:12 +0100

x86/boot: Standardize strcmp()

strcmp() is always expected to return 0 when arguments are equal,
negative when its first argument @str1 is less than its second argument
@str2 and a positive value otherwise. Previously strcmp("a", "b")
returned 1. Now it gives -1, as it is supposed to.

Until now this bug never triggered, because all uses for strcmp() in the
boot code tested for nonzero:

  triton:~/tip> git grep strcmp arch/x86/boot/
  arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
  arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))

should in the future strcmp() be used in a comparative way in the boot
code, it might have led to (not so subtle) bugs.

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1426520267-1803-1-git-send-email-arjun024@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 493f3fd..318b846 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;