Bug#403763: Error 23 on savedefault when using grub-reboot command

[Len Sorensen]

Package: grub
Version: 0.97-20
Severity: normal
Tags: patch

I am running with the current grub package compiled on a sarge system.

When I try to use the grub-reboot command to change the boot entry to
use on next boot as a one time choice, the /boot/grub/default is updated
to show 0:2 as expected.  menu.lst has 'default saved'.  When booting,
it correctly goes to entry 2, but when it hits savedefault (which should
restore the default file back to just '0', it gets an error 23 trying to
parse the value.  I eventually with a lot of debuging determined that
the 'char *def' in the savedefault_helper is somehow now making it
through when passed to safe_parse_maxint(&def, &entryno);  The following
patch fixes it for me however:

# This fixes some warnings about missing return on a function
@@ -3660,9 +3663,9 @@
 {
 #if !defined(SUPPORT_DISKLESS)
   #if !defined(GRUB_UTIL)
-       savedefault_helper(arg, flags);
+       return savedefault_helper(arg, flags);
   #else
-       savedefault_shell(arg, flags);
+       return savedefault_shell(arg, flags);
   #endif
 #else /* !SUPPORT_DISKLESS */
   errnum = ERR_UNRECOGNIZED;

# This fixes the actual problem
@@ -3829,6 +3832,7 @@

       disk_read_hook = disk_read_savesect_func;
       len = grub_read (buf, sizeof (buf));
+      buf[9]='\0'; /* Make sure grub_strstr() below terminates */
       disk_read_hook = 0;
       grub_close ();

@@ -3844,9 +3848,8 @@
       {
        int f_len = grub_strlen(buf) - grub_strlen(tmp);
        char *def;
-       int a;
-       for(a = 0; a < f_len; a++)
-         grub_memcpy(&def[a], &buf[a], sizeof(char));
+       buf[f_len] = '\0';
+       def = buf;
        safe_parse_maxint (&def, &entryno);
       }

This seems a bit simpler too.  We can reuse the buf rather than memcpy'ing
(to where?  where did def have any memory malloc'd in the first
place?) since buf is about to be completely overwritten below anyhow.
We aren't going to use tmp anymore either, so changing the contents at
tmp's location doesn't hurt either.  Passing &buf to safe_parse_maxint
causes a warning about 'invalid pointer type as argument' while assigning
the char* def and passing it, works fine.  I don't understand why,
since I thought they were supposed to be equivalant in C.  Maybe the
compiler in sarge is crappy or something.  Assigning a \0 to the end
of the buf after filling it seems safer, since it ensures grub_strstr
will hit the end of the buffer rather than walk through memory until
it eventually finds a \0 somewhere if it doesn't happen to have a :
in the string on a given pass.

-- System Information:
Debian Release: 3.1
Architecture: i386 (x86_64)
Kernel: Linux 2.6.11-9-amd64-k8
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)

Versions of packages grub depends on:
ii  libc6                 2.3.2.ds1-22sarge4 GNU C Library: Shared libraries an
ii  libncurses5           5.4-4              Shared libraries for terminal hand

-- no debconf information