Bug in read_handler_list (autoloading) ???

Bug in read_handler_list (autoloading) ???

[BVK]

Hi,


I was trying grub-emu from experimental branch, and grub-emu aborts
with below error.  It seems read_handler_list function is trying to
grub_free non-heap global objects.


Suspecious code (read_handler_list in normal/handler.c):

              /* Override previous handler.lst.  */
              while (grub_handler_class_list)
                {
                  grub_handler_class_t tmp;
                  tmp = grub_handler_class_list->next;
                  grub_free (grub_handler_class_list);
                  grub_handler_class_list = tmp;
                }


Error message:

bvk@ubook:~/sid-home/foss/grub2/bzr/b$ sudo ./grub-emu >foo.txt 2>&1
*** glibc detected *** ./grub-emu: free(): invalid pointer: 0x08095860 ***
======= Backtrace: =========
/lib/tls/i686/cmov/libc.so.6[0x448ff1]
/lib/tls/i686/cmov/libc.so.6[0x44a6f2]
/lib/tls/i686/cmov/libc.so.6(cfree+0x6d)[0x44d79d]
./grub-emu[0x80875c4]
./grub-emu[0x805e4e4]
./grub-emu[0x806080f]
./grub-emu[0x8060852]
./grub-emu[0x806090b]
./grub-emu[0x806098b]
./grub-emu[0x8059d49]
./grub-emu[0x8059d11]
./grub-emu[0x8059de0]
./grub-emu[0x8086fa0]
/lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe6)[0x3f4b56]
./grub-emu[0x80496f1]
======= Memory map: ========
0011f000-0013b000 r-xp 00000000 08:03 750        /lib/libgcc_s.so.1
0013b000-0013c000 r-xp 0001b000 08:03 750        /lib/libgcc_s.so.1
0013c000-0013d000 rwxp 0001c000 08:03 750        /lib/libgcc_s.so.1
0021d000-00238000 r-xp 00000000 08:03 700        /lib/ld-2.10.1.so
00238000-00239000 r-xp 0001a000 08:03 700        /lib/ld-2.10.1.so
00239000-0023a000 rwxp 0001b000 08:03 700        /lib/ld-2.10.1.so
003c1000-003c2000 rwxp 00000000 00:00 0
003de000-0051c000 r-xp 00000000 08:03 1565
/lib/tls/i686/cmov/libc-2.10.1.so
0051c000-0051e000 r-xp 0013e000 08:03 1565
/lib/tls/i686/cmov/libc-2.10.1.so
0051e000-0051f000 rwxp 00140000 08:03 1565
/lib/tls/i686/cmov/libc-2.10.1.so
0051f000-00522000 rwxp 00000000 00:00 0
0074f000-00751000 rwxp 00000000 00:00 0
008f4000-008f6000 r-xp 00000000 08:03 1571
/lib/tls/i686/cmov/libdl-2.10.1.so
008f6000-008f7000 r-xp 00001000 08:03 1571
/lib/tls/i686/cmov/libdl-2.10.1.so
008f7000-008f8000 rwxp 00002000 08:03 1571
/lib/tls/i686/cmov/libdl-2.10.1.so
00b89000-00b8a000 rwxp 00000000 00:00 0
00c98000-00ccc000 r-xp 00000000 08:03 770        /lib/libncurses.so.5.7
00ccc000-00ccd000 ---p 00034000 08:03 770        /lib/libncurses.so.5.7
00ccd000-00ccf000 r-xp 00034000 08:03 770        /lib/libncurses.so.5.7
00ccf000-00cd0000 rwxp 00036000 08:03 770        /lib/libncurses.so.5.7
00dfa000-00dfb000 r-xp 00000000 00:00 0          [vdso]
08048000-08095000 r-xp 00000000 08:03 58479
/home/bvk/sid-home/foss/grub2/bzr/b/grub-emu
08095000-08097000 rwxp 0004c000 08:03 58479
/home/bvk/sid-home/foss/grub2/bzr/b/grub-emu
08097000-080bf000 rwxp 00000000 00:00 0
08f82000-08fbb000 rwxp 00000000 00:00 0          [heap]
b7700000-b7721000 rwxp 00000000 00:00 0
b7721000-b7800000 ---p 00000000 00:00 0
bf85a000-bf86f000 rwxp 00000000 00:00 0          [stack]
Aborted




-- 
bvk.chaitanya

Re: Bug in read_handler_list (autoloading) ???

[BVK]

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

Valgrind log is attached. It also reported invalid free for the same place.

BTW, valgrind is run as

    sudo valgrind -v --log-file=/tmp/valgrind.log ./grub-emu



-- 
bvk.chaitanya

[-- Attachment #2: valgrind.log --]
[-- Type: application/octet-stream, Size: 6786 bytes --]

==1475== Memcheck, a memory error detector
==1475== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==1475== Using Valgrind-3.5.0-Debian and LibVEX; rerun with -h for copyright info
==1475== Command: ./grub-emu
==1475== Parent PID: 15560
==1475== 
--1475-- 
--1475-- Valgrind options:
--1475--    --suppressions=/usr/lib/valgrind/debian-libc6-dbg.supp
--1475--    -v
--1475--    --log-file=/tmp/valgrind.log
--1475-- Contents of /proc/version:
--1475--   Linux version 2.6.31-14-generic (buildd@rothera) (gcc version 4.4.1 (Ubuntu 4.4.1-4ubuntu8) ) #48-Ubuntu SMP Fri Oct 16 14:04:26 UTC 2009
--1475-- Arch and hwcaps: X86, x86-sse1-sse2
--1475-- Page sizes: currently 4096, max supported 4096
--1475-- Valgrind library directory: /usr/lib/valgrind
--1475-- Reading syms from /lib/ld-2.10.1.so (0x4000000)
--1475-- Reading debug info from /lib/ld-2.10.1.so ..
--1475-- .. CRC mismatch (computed 87a7f164 wanted cece0b9f)
--1475-- Reading debug info from /usr/lib/debug/lib/ld-2.10.1.so ..
--1475-- Reading syms from /home/bvk/sid-home/foss/grub2/bzr/experimental/b/grub-emu (0x8048000)
--1475-- Reading syms from /usr/lib/valgrind/memcheck-x86-linux (0x38000000)
--1475--    object doesn't have a dynamic symbol table
--1475-- Reading suppressions file: /usr/lib/valgrind/debian-libc6-dbg.supp
--1475-- Reading suppressions file: /usr/lib/valgrind/default.supp
--1475-- REDIR: 0x4015ed0 (index) redirected to 0x3803e013 (vgPlain_x86_linux_REDIR_FOR_index)
--1475-- Reading syms from /usr/lib/valgrind/vgpreload_core-x86-linux.so (0x401f000)
--1475-- Reading syms from /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so (0x4022000)
==1475== WARNING: new redirection conflicts with existing -- ignoring it
--1475--     new: 0x04015ed0 (index               ) R-> 0x040258c0 index
--1475-- REDIR: 0x4016080 (strlen) redirected to 0x4025bb0 (strlen)
--1475-- Reading syms from /lib/libncurses.so.5.7 (0x4039000)
--1475-- Reading debug info from /lib/libncurses.so.5.7 ..
--1475-- .. CRC mismatch (computed 5f9c3f42 wanted edb54274)
--1475--    object doesn't have a symbol table
--1475-- Reading syms from /lib/tls/i686/cmov/libc-2.10.1.so (0x4071000)
--1475-- Reading debug info from /lib/tls/i686/cmov/libc-2.10.1.so ..
--1475-- .. CRC mismatch (computed 74ff2163 wanted e27b476b)
--1475-- Reading debug info from /usr/lib/debug/lib/tls/i686/cmov/libc-2.10.1.so ..
--1475-- Reading syms from /lib/tls/i686/cmov/libdl-2.10.1.so (0x41b6000)
--1475-- Reading debug info from /lib/tls/i686/cmov/libdl-2.10.1.so ..
--1475-- .. CRC mismatch (computed f2aaa5ba wanted 04923e38)
--1475-- Reading debug info from /usr/lib/debug/lib/tls/i686/cmov/libdl-2.10.1.so ..
--1475-- REDIR: 0x40e42c0 (rindex) redirected to 0x40257d0 (rindex)
--1475-- REDIR: 0x40e3f10 (strlen) redirected to 0x4025b90 (strlen)
--1475-- REDIR: 0x40e40f0 (strncmp) redirected to 0x4025e40 (strncmp)
--1475-- REDIR: 0x40dff60 (calloc) redirected to 0x4023ea7 (calloc)
--1475-- REDIR: 0x40e3840 (index) redirected to 0x4025860 (index)
--1475-- REDIR: 0x40e0810 (malloc) redirected to 0x4024b97 (malloc)
--1475-- REDIR: 0x40e8900 (strchrnul) redirected to 0x4026c50 (strchrnul)
--1475-- REDIR: 0x40e5fc0 (memcpy) redirected to 0x4026010 (memcpy)
--1475-- REDIR: 0x40e0730 (free) redirected to 0x40247b1 (free)
--1475-- REDIR: 0x40e41e0 (strncpy) redirected to 0x4025cd0 (strncpy)
--1475-- REDIR: 0x40e39b0 (strcmp) redirected to 0x4025eb0 (strcmp)
--1475-- REDIR: 0x40e1750 (realloc) redirected to 0x4024c46 (realloc)
--1475-- REDIR: 0x40e3a20 (strcpy) redirected to 0x4025bf0 (strcpy)
--1475-- REDIR: 0x40e55d0 (memchr) redirected to 0x4025fd0 (memchr)
--1475-- REDIR: 0x40e5b30 (mempcpy) redirected to 0x4026ca0 (mempcpy)
==1475== Warning: noted but unhandled ioctl 0x1261 with no size/direction hints
==1475==    This could cause spurious value errors to appear.
==1475==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==1475== Warning: noted but unhandled ioctl 0x1261 with no size/direction hints
==1475==    This could cause spurious value errors to appear.
==1475==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==1475== Warning: noted but unhandled ioctl 0x1261 with no size/direction hints
==1475==    This could cause spurious value errors to appear.
==1475==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==1475== Invalid free() / delete / delete[]
==1475==    at 0x4024836: free (vg_replace_malloc.c:325)
==1475==    by 0x8087593: grub_free (misc.c:273)
==1475==    by 0x805E4B5: read_handler_list (handler.c:185)
==1475==    by 0x80607DE: read_lists (main.c:413)
==1475==    by 0x8060821: grub_normal_execute (main.c:424)
==1475==    by 0x80608DA: grub_enter_normal_mode (main.c:453)
==1475==    by 0x806095A: grub_cmd_normal (main.c:478)
==1475==    by 0x8059D48: grub_command_execute (command.h:115)
==1475==    by 0x8059D10: grub_load_normal_mode (main.c:145)
==1475==    by 0x8059DDF: grub_main (main.c:175)
==1475==    by 0x8086F6F: main (grub-emu.c:236)
==1475==  Address 0x8095720 is 0 bytes inside data symbol "grub_parser_class"
==1475== 
==1475== 
==1475== HEAP SUMMARY:
==1475==     in use at exit: 272,407 bytes in 344 blocks
==1475==   total heap usage: 3,232 allocs, 2,892 frees, 9,088,423 bytes allocated
==1475== 
==1475== Searching for pointers to 344 not-freed blocks
==1475== Checked 494,132 bytes
==1475== 
==1475== LEAK SUMMARY:
==1475==    definitely lost: 5,639 bytes in 30 blocks
==1475==    indirectly lost: 92 bytes in 6 blocks
==1475==      possibly lost: 0 bytes in 0 blocks
==1475==    still reachable: 266,676 bytes in 308 blocks
==1475==         suppressed: 0 bytes in 0 blocks
==1475== Rerun with --leak-check=full to see details of leaked memory
==1475== 
==1475== ERROR SUMMARY: 4 errors from 1 contexts (suppressed: 17 from 8)
==1475== 
==1475== 4 errors in context 1 of 1:
==1475== Invalid free() / delete / delete[]
==1475==    at 0x4024836: free (vg_replace_malloc.c:325)
==1475==    by 0x8087593: grub_free (misc.c:273)
==1475==    by 0x805E4B5: read_handler_list (handler.c:185)
==1475==    by 0x80607DE: read_lists (main.c:413)
==1475==    by 0x8060821: grub_normal_execute (main.c:424)
==1475==    by 0x80608DA: grub_enter_normal_mode (main.c:453)
==1475==    by 0x806095A: grub_cmd_normal (main.c:478)
==1475==    by 0x8059D48: grub_command_execute (command.h:115)
==1475==    by 0x8059D10: grub_load_normal_mode (main.c:145)
==1475==    by 0x8059DDF: grub_main (main.c:175)
==1475==    by 0x8086F6F: main (grub-emu.c:236)
==1475==  Address 0x8095720 is 0 bytes inside data symbol "grub_parser_class"
==1475== 
--1475-- 
--1475-- used_suppression:     17 dl-hack3-cond-1
==1475== 
==1475== ERROR SUMMARY: 4 errors from 1 contexts (suppressed: 17 from 8)

[PATCH] Re: Bug in read_handler_list (autoloading) ???

[Felix Zielcke]

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

Am Donnerstag, den 12.11.2009, 21:46 +0530 schrieb BVK:
> Valgrind log is attached. It also reported invalid free for the same place.
> 
> BTW, valgrind is run as
> 
>     sudo valgrind -v --log-file=/tmp/valgrind.log ./grub-emu

Here's a grub-emu tested patch to fix this



-- 
Felix Zielcke
Proud Debian Maintainer and GNU GRUB developer

[-- Attachment #2: fix_prefix_patch.diff --]
[-- Type: text/x-patch, Size: 1129 bytes --]

2009-11-12  Felix Zielcke  <fzielcke@z-51.de>

	* normal/handler.c (read_handler_list): Use grub_handler_unregister
	to properly remove the handlers.  Skip `rescue' and `console'
	handlers defined in kernel.

=== modified file 'normal/handler.c'
--- normal/handler.c	2009-11-09 14:55:27 +0000
+++ normal/handler.c	2009-11-12 20:31:00 +0000
@@ -176,14 +176,21 @@ read_handler_list (void)
 	  if (file)
 	    {
 	      char *buf = NULL;
-
+	      grub_handler_class_t hcnext, hc = grub_handler_class_list;
+	      grub_handler_t hl;
 	      /* Override previous handler.lst.  */
-	      while (grub_handler_class_list)
+	      for (; hc ; hc = hcnext)
 		{
-		  grub_handler_class_t tmp;
-		  tmp = grub_handler_class_list->next;
-		  grub_free (grub_handler_class_list);
-		  grub_handler_class_list = tmp;
+		  hcnext = hc->next;
+		  hl = hc->handler_list;
+		  while (hl)
+		    {
+		      grub_handler_t tmp = hl->next;
+		      if (grub_strcmp (hl->name, "rescue") != 0
+			  && grub_strcmp (hl->name, "console") != 0)
+			grub_handler_unregister (hc,hl);
+		      hl = tmp;
+		    }
 		}
 
 	      for (;; grub_free (buf))

Re: [PATCH] Re: Bug in read_handler_list (autoloading) ???

[Robert Millan]

Nice work.

On Thu, Nov 12, 2009 at 09:36:34PM +0100, Felix Zielcke wrote:
>  	      /* Override previous handler.lst.  */
> -	      while (grub_handler_class_list)
> +	      for (; hc ; hc = hcnext)
>  		{
> -		  grub_handler_class_t tmp;
> -		  tmp = grub_handler_class_list->next;
> -		  grub_free (grub_handler_class_list);
> -		  grub_handler_class_list = tmp;
> +		  hcnext = hc->next;
> +		  hl = hc->handler_list;
> +		  while (hl)
> +		    {
> +		      grub_handler_t tmp = hl->next;
> +		      if (grub_strcmp (hl->name, "rescue") != 0
> +			  && grub_strcmp (hl->name, "console") != 0)

Could you add a comment here explaining why these are skipped?

The rest is OK as far as I'm concerned.

-- 
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: [PATCH] Re: Bug in read_handler_list (autoloading) ???

[Vladimir 'phcoder' Serbinenko]

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

Felix Zielcke wrote:
> Am Donnerstag, den 12.11.2009, 21:46 +0530 schrieb BVK:
>   
>> Valgrind log is attached. It also reported invalid free for the same place.
>>
>> BTW, valgrind is run as
>>
>>     sudo valgrind -v --log-file=/tmp/valgrind.log ./grub-emu
>>     
>
> Here's a grub-emu tested patch to fix this
>
>
>   
Why are all handlers are removed on normal.mod unload? If user e.g.
relies on special terminal he may lose terminal completely. I would say
it's better not to touch already loaded modules. I temporarily unmerged
it from experimental. We can remerge it once issues are fixed
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: [PATCH] Re: Bug in read_handler_list (autoloading) ???

[Robert Millan]

On Fri, Nov 13, 2009 at 11:43:52PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Felix Zielcke wrote:
> > Am Donnerstag, den 12.11.2009, 21:46 +0530 schrieb BVK:
> >   
> >> Valgrind log is attached. It also reported invalid free for the same place.
> >>
> >> BTW, valgrind is run as
> >>
> >>     sudo valgrind -v --log-file=/tmp/valgrind.log ./grub-emu
> >>     
> >
> > Here's a grub-emu tested patch to fix this
> >
> >
> >   
> Why are all handlers are removed on normal.mod unload? If user e.g.
> relies on special terminal he may lose terminal completely. I would say
> it's better not to touch already loaded modules. I temporarily unmerged
> it from experimental. We can remerge it once issues are fixed

Did you unmerge Felix' fix, or the original prefix move fix?

My goal was to make command and file-system auto-completion not break when
$prefix is changed.  Handlers are not the important change in my initial
patch, and if it's problematic it can be discarded IMO.

-- 
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: [PATCH] Re: Bug in read_handler_list (autoloading) ???

[Felix Zielcke]

Am Samstag, den 14.11.2009, 16:35 +0100 schrieb Robert Millan:
> On Fri, Nov 13, 2009 at 11:43:52PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > Felix Zielcke wrote:
> > > Am Donnerstag, den 12.11.2009, 21:46 +0530 schrieb BVK:
> > >   
> > >> Valgrind log is attached. It also reported invalid free for the same place.
> > >>
> > >> BTW, valgrind is run as
> > >>
> > >>     sudo valgrind -v --log-file=/tmp/valgrind.log ./grub-emu
> > >>     
> > >
> > > Here's a grub-emu tested patch to fix this
> > >
> > >
> > >   
> > Why are all handlers are removed on normal.mod unload? If user e.g.
> > relies on special terminal he may lose terminal completely. I would say
> > it's better not to touch already loaded modules. I temporarily unmerged
> > it from experimental. We can remerge it once issues are fixed
> 
> Did you unmerge Felix' fix, or the original prefix move fix?

I didn't commit my fix.
He reverted your original merge of this.

> My goal was to make command and file-system auto-completion not break when
> $prefix is changed.  Handlers are not the important change in my initial
> patch, and if it's problematic it can be discarded IMO.
> 


-- 
Felix Zielcke
Proud Debian Maintainer and GNU GRUB developer

Re: [PATCH] Re: Bug in read_handler_list (autoloading) ???

[Robert Millan]

On Sat, Nov 14, 2009 at 10:11:31PM +0100, Felix Zielcke wrote:
> Am Samstag, den 14.11.2009, 16:35 +0100 schrieb Robert Millan:
> > On Fri, Nov 13, 2009 at 11:43:52PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > > Why are all handlers are removed on normal.mod unload? If user e.g.
> > > relies on special terminal he may lose terminal completely. I would say
> > > it's better not to touch already loaded modules. I temporarily unmerged
> > > it from experimental. We can remerge it once issues are fixed
> > 
> > Did you unmerge Felix' fix, or the original prefix move fix?
> 
> I didn't commit my fix.
> He reverted your original merge of this.

Ok, then if there are no objections I'll remerge without the handler bits.

-- 
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: [PATCH] Re: Bug in read_handler_list (autoloading) ???

[Robert Millan]

On Sun, Nov 15, 2009 at 12:10:24PM +0100, Robert Millan wrote:
> On Sat, Nov 14, 2009 at 10:11:31PM +0100, Felix Zielcke wrote:
> > Am Samstag, den 14.11.2009, 16:35 +0100 schrieb Robert Millan:
> > > On Fri, Nov 13, 2009 at 11:43:52PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > > > Why are all handlers are removed on normal.mod unload? If user e.g.
> > > > relies on special terminal he may lose terminal completely. I would say
> > > > it's better not to touch already loaded modules. I temporarily unmerged
> > > > it from experimental. We can remerge it once issues are fixed
> > > 
> > > Did you unmerge Felix' fix, or the original prefix move fix?
> > 
> > I didn't commit my fix.
> > He reverted your original merge of this.
> 
> Ok, then if there are no objections I'll remerge without the handler bits.

Merged into trunk now, without the handler bits and with command.lst fixes by
Vladimir.  Thanks to all for your feedback on resolving this.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi