All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86,boot: standardize strcmp()
@ 2015-03-16 15:37 Arjun Sreedharan
  2015-03-16 18:16 ` Borislav Petkov
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-16 15:37 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Thomas Gleixner, Ingo Molnar, x86, linux-kernel

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
@ 2015-03-16 18:16 ` Borislav Petkov
  2015-03-17  7:46 ` Ingo Molnar
  2015-03-23 12:23 ` [tip:x86/boot] x86/boot: Standardize strcmp() tip-bot for Arjun Sreedharan
  2 siblings, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2015-03-16 18:16 UTC (permalink / raw)
  To: Arjun Sreedharan
  Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-kernel

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.
--

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
  2015-03-16 18:16 ` Borislav Petkov
@ 2015-03-17  7:46 ` Ingo Molnar
  2015-03-17 14:13   ` Arjun Sreedharan
  2015-03-17 14:28   ` Borislav Petkov
  2015-03-23 12:23 ` [tip:x86/boot] x86/boot: Standardize strcmp() tip-bot for Arjun Sreedharan
  2 siblings, 2 replies; 11+ messages in thread
From: Ingo Molnar @ 2015-03-17  7:46 UTC (permalink / raw)
  To: Arjun Sreedharan
  Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-kernel


* 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-17  7:46 ` Ingo Molnar
@ 2015-03-17 14:13   ` Arjun Sreedharan
  2015-03-18  1:36     ` Bernd Petrovitsch
  2015-03-19 17:34     ` H. Peter Anvin
  2015-03-17 14:28   ` Borislav Petkov
  1 sibling, 2 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-17 14:13 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-17  7:46 ` Ingo Molnar
  2015-03-17 14:13   ` Arjun Sreedharan
@ 2015-03-17 14:28   ` Borislav Petkov
  1 sibling, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2015-03-17 14:28 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Arjun Sreedharan, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
	x86, linux-kernel

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.
--

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-17 14:13   ` Arjun Sreedharan
@ 2015-03-18  1:36     ` Bernd Petrovitsch
  2015-03-18 18:10       ` Arjun Sreedharan
  2015-03-18 18:37       ` Arjun Sreedharan
  2015-03-19 17:34     ` H. Peter Anvin
  1 sibling, 2 replies; 11+ messages in thread
From: Bernd Petrovitsch @ 2015-03-18  1:36 UTC (permalink / raw)
  To: Arjun Sreedharan
  Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-18  1:36     ` Bernd Petrovitsch
@ 2015-03-18 18:10       ` Arjun Sreedharan
  2015-03-18 18:37       ` Arjun Sreedharan
  1 sibling, 0 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-18 18:10 UTC (permalink / raw)
  To: Bernd Petrovitsch
  Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86,
	linux-kernel

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
>

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] x86,boot: standardize strcmp()
  2015-03-18  1:36     ` Bernd Petrovitsch
  2015-03-18 18:10       ` Arjun Sreedharan
@ 2015-03-18 18:37       ` Arjun Sreedharan
  1 sibling, 0 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-18 18:37 UTC (permalink / raw)
  To: Bernd Petrovitsch
  Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86,
	linux-kernel

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++;
 	}
-- 


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-17 14:13   ` Arjun Sreedharan
  2015-03-18  1:36     ` Bernd Petrovitsch
@ 2015-03-19 17:34     ` H. Peter Anvin
  2015-03-20 11:42       ` Bernd Petrovitsch
  1 sibling, 1 reply; 11+ messages in thread
From: H. Peter Anvin @ 2015-03-19 17:34 UTC (permalink / raw)
  To: Arjun Sreedharan, Ingo Molnar
  Cc: Thomas Gleixner, Ingo Molnar, x86, linux-kernel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86,boot: standardize strcmp()
  2015-03-19 17:34     ` H. Peter Anvin
@ 2015-03-20 11:42       ` Bernd Petrovitsch
  0 siblings, 0 replies; 11+ messages in thread
From: Bernd Petrovitsch @ 2015-03-20 11:42 UTC (permalink / raw)
  To: Arjun Sreedharan
  Cc: H. Peter Anvin, Ingo Molnar, Thomas Gleixner, Ingo Molnar, x86,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tip:x86/boot] x86/boot: Standardize strcmp()
  2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
  2015-03-16 18:16 ` Borislav Petkov
  2015-03-17  7:46 ` Ingo Molnar
@ 2015-03-23 12:23 ` tip-bot for Arjun Sreedharan
  2 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Arjun Sreedharan @ 2015-03-23 12:23 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: hpa, arjun024, mingo, linux-kernel, bp, tglx

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++;

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-03-23 12:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
2015-03-16 18:16 ` Borislav Petkov
2015-03-17  7:46 ` Ingo Molnar
2015-03-17 14:13   ` Arjun Sreedharan
2015-03-18  1:36     ` Bernd Petrovitsch
2015-03-18 18:10       ` Arjun Sreedharan
2015-03-18 18:37       ` Arjun Sreedharan
2015-03-19 17:34     ` H. Peter Anvin
2015-03-20 11:42       ` Bernd Petrovitsch
2015-03-17 14:28   ` Borislav Petkov
2015-03-23 12:23 ` [tip:x86/boot] x86/boot: Standardize strcmp() tip-bot for Arjun Sreedharan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.