Handle C symbols with leading underscore (HAVE_ASM_USCORE)

PATCH In review Christian Franke 2 participants 5 messages Nov 10, 2007 – Nov 18, 2007

lore

Share & tools

lore

Christian Franke

author

2007-11-10 19:04 UTC #0 lore raw

PATCH 1/1 Handle C symbols with leading underscore (HAVE_ASM_USCORE)

HAVE_ASM_USCORE is checked by configure, but module loader does not work in this case.

This patch fixes this by ignoring the difference. It actually works to load modules compiled on Linux by a kernel compiled on Cygwin (with underscores) and vice versa.

It also prepares the grub_mod_init/finit search for modules converted from non-ELF format (which do not support the "function" symbol type).

A bug in symbol table scan is fixed.

Christian

2007-11-10 Christian Franke <[email protected]>

* genkernsyms.sh.in: Handle HAVE_ASM_USCORE case.

* genmk.rb: Handle HAVE_ASM_USCORE case in strip command. Add $EXEEXT to CLEANFILES.

* kern/dl.c (grub_dl_resolve_symbol): Handle HAVE_ASM_USCORE case, ignore leading underscore.
(grub_dl_register_symbol): Likewise. (grub_dl_resolve_symbols): Likewise.
Add check for grub_mod_init/fini for symbols without type. (grub_dl_resolve_dependencies): Add check for trailing nullbytes in symbol table. This fixes an infinite loop if stab is zero filled.

genkernsyms.sh.in +5 -2

diff -rup grub2.orig/genkernsyms.sh.in grub2/genkernsyms.sh.in
@@ -16,9 +16,12 @@
 srcdir=@srcdir@
 CC="@CC@"
 
+u=
+grep "^#define HAVE_ASM_USCORE" config.h >/dev/null 2>&1 && u="_"
+
 $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Iinclude -I$srcdir/include $* \
   | grep -v '^#' \
   | sed -n \
-        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
-        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
+        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
+        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
   | sort -u

genmk.rb +2 -2

diff -rup grub2.orig/genmk.rb grub2/genmk.rb
@@ -115,7 +115,7 @@ UNDSYMFILES += #{undsym}
 #{@name}: #{pre_obj} #{mod_obj}
 	-rm -f $@
 	$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) -Wl,-r,-d -o $@ $^
-	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -R .note -R .comment $@
+	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -K _grub_mod_init -K _grub_mod_fini -R .note -R .comment $@
 
 #{pre_obj}: $(#{prefix}_DEPENDENCIES) #{objs_str}
 	-rm -f $@

@@ -187,7 +187,7 @@ class Utility
     deps = objs.collect {|obj| obj.suffix('d')}
     deps_str = deps.join(' ');
 
-    "CLEANFILES += #{@name} #{objs_str}
+    "CLEANFILES += #{@name}$(EXEEXT) #{objs_str}
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: $(#{prefix}_DEPENDENCIES) #{objs_str}

kern/dl.c +28 -7

diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
@@ -156,6 +156,9 @@ grub_dl_resolve_symbol (const char *name
 {
   grub_symbol_t sym;
 
+  if (*name == '_')
+    name++;
+
   for (sym = grub_symtab[grub_symbol_hash (name)]; sym; sym = sym->next)
     if (grub_strcmp (sym->name, name) == 0)
       return sym->addr;

@@ -169,6 +172,11 @@ grub_dl_register_symbol (const char *nam
 {
   grub_symbol_t sym;
   unsigned k;
+
+  /* Ignore leading underscore used for C symbols.
+     Done at runtime to allow loading modules compiled on other OS.  */
+  if (*name == '_')
+    name++;
   
   sym = (grub_symbol_t) grub_malloc (sizeof (*sym));
   if (! sym)

@@ -347,6 +355,7 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
       unsigned char type = ELF_ST_TYPE (sym->st_info);
       unsigned char bind = ELF_ST_BIND (sym->st_info);
       const char *name = str + sym->st_name;
+      int check_mod_func = 0;
       
       switch (type)
 	{

@@ -359,6 +368,12 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 		return grub_error (GRUB_ERR_BAD_MODULE,
 				   "the symbol `%s' not found", name);
 	    }
+	  else if (sym->st_name != 0 && bind == STB_LOCAL)
+	    { /* Static functions have no type if initial format was not ELF.  */
+	      sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
+								    sym->st_shndx);
+	      check_mod_func = 1;
+	    }
 	  else
 	    sym->st_value = 0;
 	  break;

@@ -374,14 +389,11 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	case STT_FUNC:
 	  sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
 								sym->st_shndx);
-	  if (bind != STB_LOCAL)
+	  if (bind == STB_LOCAL)
+	    check_mod_func = 1;
+	  else	
 	    if (grub_dl_register_symbol (name, (void *) sym->st_value, mod))
 	      return grub_errno;
-	  
-	  if (grub_strcmp (name, "grub_mod_init") == 0)
-	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
-	  else if (grub_strcmp (name, "grub_mod_fini") == 0)
-	    mod->fini = (void (*) (void)) sym->st_value;
 	  break;
 
 	case STT_SECTION:

@@ -397,6 +409,15 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	  return grub_error (GRUB_ERR_BAD_MODULE,
 			     "unknown symbol type `%d'", (int) type);
 	}
+      if (check_mod_func)
+        {
+	  if (*name == '_')
+	    name++;
+	  if (grub_strcmp (name, "grub_mod_init") == 0)
+	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
+	  else if (grub_strcmp (name, "grub_mod_fini") == 0)
+	    mod->fini = (void (*) (void)) sym->st_value;
+        }
     }
 
   return GRUB_ERR_NONE;

@@ -454,7 +475,7 @@ grub_dl_resolve_dependencies (grub_dl_t 
 	const char *name = (char *) e + s->sh_offset;
 	const char *max = name + s->sh_size;
 
-	while (name < max)
+	while (name < max && *name) /* Segment may contain trailing 0.  */
 	  {
 	    grub_dl_t m;
 	    grub_dl_dep_t dep;

Robert Millan

2007-11-10 21:24 UTC #1 lore raw

kern/dl.c grub_dl_register_symbol (const char *nam · line 179

Quoted 11 lines · Christian Franke

This patch fixes this by ignoring the difference. It actually works to load modules compiled on Linux by a kernel compiled on Cygwin (with underscores) and vice versa.
[...]
kern/dl.c +5 -0
+
+  /* Ignore leading underscore used for C symbols.
+     Done at runtime to allow loading modules compiled on other OS.  */
+  if (*name == '_')
+    name++;

Is that something we really want? We never made any efforts at maintaining ABI between kernel and modules, or in allowing this kind of combinations. If we go this path, it can mean more work later (e.g. support for building kernel and modules with different versions of GCC, etc).

Could you give an example of a situation in which this would be useful?

Signature · 5 lines

Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Christian Franke

author

2007-11-11 18:25 UTC #2 lore raw

Quoted 12 lines · Robert Millan

Review
Quoted 9 lines · Christian Franke
This patch fixes this by ignoring the difference. It actually works to load modules compiled on Linux by a kernel compiled on Cygwin (with underscores) and vice versa.
[...]
(quoted diff) +5 -0
+
+  /* Ignore leading underscore used for C symbols.
+     Done at runtime to allow loading modules compiled on other OS.  */
+  if (*name == '_')
+    name++;

Quoted 6 lines

Is that something we really want? We never made any efforts at maintaining ABI between kernel and modules, or in allowing this kind of combinations. If we go this path, it can mean more work later (e.g. support for building kernel and modules with different versions of GCC, etc).

Agree.

Should the kernel probably check whether a module is build for the same target? This would avoid interesting behaviour if a foreign module is accidentally load for some reason. It could e.g. be done by checking a symbol derived from $target.

Quoted 3 lines

Could you give an example of a situation in which this would be useful?

There probably is none, therefore the (older) patch for the other variant is attached :-)

Christian

2007-11-11 Christian Franke <[email protected]>

* genkernsyms.sh.in: Handle HAVE_ASM_USCORE case.

* genmk.rb: Handle HAVE_ASM_USCORE case in strip command. Add EXEEXT to CLEANFILES.

* gensymlist.sh.in: Handle HAVE_ASM_USCORE case.

* kern/dl.c (grub_dl_resolve_symbols): Add check for grub_mod_init and grub_mod_fini for symbols without a type. Handle HAVE_ASM_USCORE case for these symbols.
(grub_dl_resolve_dependencies): Add check for trailing nullbytes in symbol table. This fixes an infinite loop if table is zero filled.

genkernsyms.sh.in +5 -2

diff -rup grub2.orig/genkernsyms.sh.in grub2/genkernsyms.sh.in
@@ -16,9 +16,12 @@
 srcdir=@srcdir@
 CC="@CC@"
 
+u=
+grep "^#define HAVE_ASM_USCORE" config.h >/dev/null 2>&1 && u="_"
+
 $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Iinclude -I$srcdir/include $* \
   | grep -v '^#' \
   | sed -n \
-        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
-        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
+        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
+        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
   | sort -u

genmk.rb +2 -2

diff -rup grub2.orig/genmk.rb grub2/genmk.rb
@@ -115,7 +115,7 @@ UNDSYMFILES += #{undsym}
 #{@name}: #{pre_obj} #{mod_obj}
 	-rm -f $@
 	$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) -Wl,-r,-d -o $@ $^
-	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -R .note -R .comment $@
+	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -K _grub_mod_init -K _grub_mod_fini -R .note -R .comment $@
 
 #{pre_obj}: $(#{prefix}_DEPENDENCIES) #{objs_str}
 	-rm -f $@

@@ -187,7 +187,7 @@ class Utility
     deps = objs.collect {|obj| obj.suffix('d')}
     deps_str = deps.join(' ');
 
-    "CLEANFILES += #{@name} #{objs_str}
+    "CLEANFILES += #{@name}$(EXEEXT) #{objs_str}
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: $(#{prefix}_DEPENDENCIES) #{objs_str}

gensymlist.sh.in +4 -2

diff -rup grub2.orig/gensymlist.sh.in grub2/gensymlist.sh.in
@@ -16,6 +16,8 @@
 srcdir=@srcdir@
 CC=@CC@
 
+u=
+grep "^#define HAVE_ASM_USCORE" config.h >/dev/null 2>&1 && u="_"
 
 cat <<EOF
 /* This file is automatically generated by gensymlist.sh. DO NOT EDIT! */

@@ -60,8 +62,8 @@ EOF
 $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Iinclude -I$srcdir/include $* \
   | grep -v '^#' \
   | sed -n \
-        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/      {"\1", \1},/;p;}' \
-        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/      {"\1", \&\1},/;p;}' \
+        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/      {"'"$u"'\1", \1},/;p;}' \
+        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/      {"'"$u"'\1", \&\1},/;p;}' \
   | sort -u
 
 cat <<EOF

kern/dl.c +24 -7

diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
@@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym;
 
 #endif
 
+#ifdef HAVE_ASM_USCORE
+# define SYM_USCORE "_"
+#else
+# define SYM_USCORE ""
+#endif
+
 
 
 struct grub_dl_list

@@ -347,6 +353,7 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
       unsigned char type = ELF_ST_TYPE (sym->st_info);
       unsigned char bind = ELF_ST_BIND (sym->st_info);
       const char *name = str + sym->st_name;
+      int check_mod_func = 0;
       
       switch (type)
 	{

@@ -359,6 +366,12 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 		return grub_error (GRUB_ERR_BAD_MODULE,
 				   "the symbol `%s' not found", name);
 	    }
+	  else if (sym->st_name != 0 && bind == STB_LOCAL)
+	    { /* Static functions have no type if initial format was not ELF.  */
+	      sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
+								    sym->st_shndx);
+	      check_mod_func = 1;
+	    }
 	  else
 	    sym->st_value = 0;
 	  break;

@@ -374,14 +387,11 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	case STT_FUNC:
 	  sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
 								sym->st_shndx);
-	  if (bind != STB_LOCAL)
+	  if (bind == STB_LOCAL)
+	    check_mod_func = 1;
+	  else	
 	    if (grub_dl_register_symbol (name, (void *) sym->st_value, mod))
 	      return grub_errno;
-	  
-	  if (grub_strcmp (name, "grub_mod_init") == 0)
-	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
-	  else if (grub_strcmp (name, "grub_mod_fini") == 0)
-	    mod->fini = (void (*) (void)) sym->st_value;
 	  break;
 
 	case STT_SECTION:

@@ -397,6 +407,13 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	  return grub_error (GRUB_ERR_BAD_MODULE,
 			     "unknown symbol type `%d'", (int) type);
 	}
+      if (check_mod_func)
+        {
+	  if (grub_strcmp (name, SYM_USCORE "grub_mod_init") == 0)
+	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
+	  else if (grub_strcmp (name, SYM_USCORE "grub_mod_fini") == 0)
+	    mod->fini = (void (*) (void)) sym->st_value;
+        }
     }
 
   return GRUB_ERR_NONE;

@@ -454,7 +471,7 @@ grub_dl_resolve_dependencies (grub_dl_t 
 	const char *name = (char *) e + s->sh_offset;
 	const char *max = name + s->sh_size;
 
-	while (name < max)
+	while (name < max && *name) /* Segment may contain trailing 0.  */
 	  {
 	    grub_dl_t m;
 	    grub_dl_dep_t dep;

Robert Millan

2007-11-18 06:19 UTC #3 lore raw

Quoted 5 lines · Christian Franke

Should the kernel probably check whether a module is build for the same target? This would avoid interesting behaviour if a foreign module is accidentally load for some reason. It could e.g. be done by checking a symbol derived from $target.

It's important to keep the kernel small. IMHO checks against improper use themselves don't justify increasing its size (although what you want might fit well in grub-mkimage, at least for modules that are put in core.img).

OTOH, there's an ongoing discussion on how to embed a filesystem image as part of core.img, which (maybe, I'm not sure ;-)) could qualify as "foreign module" and somehow collide with this. Thread subject is:
"Re: grub-setup: error: Non-sector-aligned data is found in the core file"

kern/dl.c typedef Elf64_Sym Elf_Sym; · line 60

Quoted 12 lines

diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c

kern/dl.c +5 -0

@@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym;
 
 #endif
 
+#ifdef HAVE_ASM_USCORE
+# define SYM_USCORE "_"
+#else
+# define SYM_USCORE ""
+#endif

Should this be global? We already have START_SYMBOL and END_SYMBOL. Perhaps this should be next to them?

Signature · 5 lines

Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Christian Franke

author

2007-11-18 20:52 UTC #4 lore raw

Quoted 8 lines · Robert Millan

Quoted 8 lines
diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
kern/dl.c +0 -0
@@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym;
 
 #endif
 

Quoted 11 lines

kern/dl.c typedef Elf64_Sym Elf_Sym; · line 62
Quoted 6 lines
kern/dl.c +5 -0
+#ifdef HAVE_ASM_USCORE
+# define SYM_USCORE "_"
+#else
+# define SYM_USCORE ""
+#endif
 
Should this be global? We already have START_SYMBOL and END_SYMBOL. Perhaps this should be next to them?

START_SYMBOL and END_SYMBOL are in config.h. SYM_USCORE is derived from a config.h symbol and therefore cannot be placed there without changing configure.ac.

The symbol is (and likely will be) only used once, so it IMO makes no sense to declare it non-locally.

Christian