On grub_strchr implementation

On grub_strchr implementation

[BVK]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

hi,


As per http://www.opengroup.org/onlinepubs/009695399/functions/strchr.html
strchr method should consider '\0' as part of the string, which means,
strchr should return '\0' character position when it is passed as the
character to look for.

I believe, grub_strchr is intended to mimic standard strchr semantics,
but its implementation doesn't confrom the above requirement.  Same
goes for grub_strrchr function too.  Attached is the patch to fix both
of these.


thanks,
-- 
bvk-chaitanya

[-- Attachment #2: fix-strchar.patch --]
[-- Type: application/octet-stream, Size: 861 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2009-10-30 22:51:52 +0000
+++ ChangeLog	2009-11-01 18:39:54 +0000
@@ -1,3 +1,8 @@
+2009-11-02  BVK Chaitanya  <bvk.groups@gmail.com>
+
+	* kern/misc.c: Fixed grub_strchr and grub_strrchr functions for
+	'\0' case.
+
 2009-10-30  Robert Millan  <rmh.grub@aybabtu.com>
 
 	Fix build problem.

=== modified file 'kern/misc.c'
--- kern/misc.c	2009-08-24 19:40:40 +0000
+++ kern/misc.c	2009-11-01 18:49:08 +0000
@@ -223,12 +223,16 @@
 char *
 grub_strchr (const char *s, int c)
 {
-  while (*s)
+  if (! s)
+    return 0;
+
+  do
     {
       if (*s == c)
 	return (char *) s;
       s++;
     }
+  while (*s);
 
   return 0;
 }
@@ -238,12 +242,15 @@
 {
   char *p = 0;
 
-  while (*s)
+  if (! s)
+    return 0;
+
+  do
     {
       if (*s == c)
 	p = (char *) s;
-      s++;
     }
+  while (*s++);
 
   return p;
 }

Re: On grub_strchr implementation

[Robert Millan]

On Mon, Nov 02, 2009 at 12:24:33AM +0530, BVK wrote:
> hi,
> 
> 
> As per http://www.opengroup.org/onlinepubs/009695399/functions/strchr.html
> strchr method should consider '\0' as part of the string, which means,
> strchr should return '\0' character position when it is passed as the
> character to look for.
> 
> I believe, grub_strchr is intended to mimic standard strchr semantics,
> but its implementation doesn't confrom the above requirement.  Same
> goes for grub_strrchr function too.  Attached is the patch to fix both
> of these.

Thanks.

But you also added a check for NULL dereference.  Is that also required by
the standard?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: On grub_strchr implementation

[BVK]

On Mon, Nov 2, 2009 at 3:28 AM, Robert Millan <rmh@aybabtu.com> wrote:
>
> But you also added a check for NULL dereference.  Is that also required by
> the standard?
>

Standard doesn't say anything about it, so I guess it means, it is
left for implementations.  Old grub_strchr implementation has this
check, so kept it as it is.  I didn't want to break any code if it is
assuming this behavior.

I just saw that other functions like, grub_strcmp, etc. don't check
for NULL dereference, so its better to remove this check.  I will send
a new patch.



-- 
bvk-chaitanya

Re: On grub_strchr implementation

[BVK]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

Attached is the new patch without NULL dereference checks.

I looked at grub_strchr and grub_strrchr calls as much as I can, and
they don't seem to rely on NULL dereference behavior.  But it was not
feasible to verify this for some cases, as we are using function
pointers (like, grub_disk_dev->open, etc.).



thanks,
-- 
bvk-chaitanya

[-- Attachment #2: fix-strchr-strrchr.patch --]
[-- Type: application/octet-stream, Size: 803 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2009-10-30 22:51:52 +0000
+++ ChangeLog	2009-11-01 18:39:54 +0000
@@ -1,3 +1,8 @@
+2009-11-02  BVK Chaitanya  <bvk.groups@gmail.com>
+
+	* kern/misc.c: Fixed grub_strchr and grub_strrchr functions for
+	'\0' case.
+
 2009-10-30  Robert Millan  <rmh.grub@aybabtu.com>
 
 	Fix build problem.

=== modified file 'kern/misc.c'
--- kern/misc.c	2009-08-24 19:40:40 +0000
+++ kern/misc.c	2009-11-02 05:41:24 +0000
@@ -223,12 +223,13 @@
 char *
 grub_strchr (const char *s, int c)
 {
-  while (*s)
+  do
     {
       if (*s == c)
 	return (char *) s;
       s++;
     }
+  while (*s);
 
   return 0;
 }
@@ -238,12 +239,12 @@
 {
   char *p = 0;
 
-  while (*s)
+  do
     {
       if (*s == c)
 	p = (char *) s;
-      s++;
     }
+  while (*s++);
 
   return p;
 }

Re: On grub_strchr implementation

[Robert Millan]

On Mon, Nov 02, 2009 at 06:21:52AM +0530, BVK wrote:
> On Mon, Nov 2, 2009 at 3:28 AM, Robert Millan <rmh@aybabtu.com> wrote:
> >
> > But you also added a check for NULL dereference.  Is that also required by
> > the standard?
> >
> 
> Standard doesn't say anything about it, so I guess it means, it is
> left for implementations.  Old grub_strchr implementation has this
> check, so kept it as it is.  I didn't want to break any code if it is
> assuming this behavior.
> 
> I just saw that other functions like, grub_strcmp, etc. don't check
> for NULL dereference, so its better to remove this check.  I will send
> a new patch.

Hi,

Please see the commit I did yesterday.  Does this suffice?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: On grub_strchr implementation

[BVK]

On Mon, Nov 2, 2009 at 7:17 PM, Robert Millan <rmh@aybabtu.com> wrote:
>
> Please see the commit I did yesterday.  Does this suffice?
>

Yes, it is sufficient.



-- 
bvk-chaitanya