* [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE)
@ 2007-11-10 19:03 Christian Franke
2007-11-10 21:24 ` Robert Millan
0 siblings, 1 reply; 5+ messages in thread
From: Christian Franke @ 2007-11-10 19:03 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]
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 <franke@computer.org>
* 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.
[-- Attachment #2: grub2-asm_uscore.patch --]
[-- Type: text/x-patch, Size: 4636 bytes --]
diff -rup grub2.orig/genkernsyms.sh.in grub2/genkernsyms.sh.in
--- grub2.orig/genkernsyms.sh.in 2006-07-12 22:42:52.000000000 +0200
+++ grub2/genkernsyms.sh.in 2007-10-06 12:59:29.000000000 +0200
@@ -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
diff -rup grub2.orig/genmk.rb grub2/genmk.rb
--- grub2.orig/genmk.rb 2007-10-20 22:49:38.968750000 +0200
+++ grub2/genmk.rb 2007-11-10 19:37:17.562500000 +0100
@@ -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}
diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
--- grub2.orig/kern/dl.c 2007-07-22 01:32:26.000000000 +0200
+++ grub2/kern/dl.c 2007-11-10 19:44:34.140625000 +0100
@@ -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;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE) 2007-11-10 19:03 [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE) Christian Franke @ 2007-11-10 21:24 ` Robert Millan 2007-11-11 18:24 ` Christian Franke 0 siblings, 1 reply; 5+ messages in thread From: Robert Millan @ 2007-11-10 21:24 UTC (permalink / raw) To: The development of GRUB 2 On Sat, Nov 10, 2007 at 08:03:57PM +0100, Christian Franke wrote: > > 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. > > [...] > + > + /* 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? -- 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 /.) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE) 2007-11-10 21:24 ` Robert Millan @ 2007-11-11 18:24 ` Christian Franke 2007-11-18 6:18 ` Robert Millan 0 siblings, 1 reply; 5+ messages in thread From: Christian Franke @ 2007-11-11 18:24 UTC (permalink / raw) To: The development of GRUB 2 [-- Attachment #1: Type: text/plain, Size: 1755 bytes --] Robert Millan wrote: > On Sat, Nov 10, 2007 at 08:03:57PM +0100, Christian Franke wrote: > >> 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. >> >> [...] >> + >> + /* 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). > > 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. > 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 <franke@computer.org> * 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. [-- Attachment #2: grub2-asm_uscore-2.patch --] [-- Type: text/x-patch, Size: 5191 bytes --] diff -rup grub2.orig/genkernsyms.sh.in grub2/genkernsyms.sh.in --- grub2.orig/genkernsyms.sh.in 2006-07-12 22:42:52.000000000 +0200 +++ grub2/genkernsyms.sh.in 2007-10-06 12:59:29.000000000 +0200 @@ -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 diff -rup grub2.orig/genmk.rb grub2/genmk.rb --- grub2.orig/genmk.rb 2007-10-20 22:49:38.968750000 +0200 +++ grub2/genmk.rb 2007-11-10 19:37:17.562500000 +0100 @@ -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} diff -rup grub2.orig/gensymlist.sh.in grub2/gensymlist.sh.in --- grub2.orig/gensymlist.sh.in 2007-07-22 01:32:18.000000000 +0200 +++ grub2/gensymlist.sh.in 2007-10-07 17:47:37.000000000 +0200 @@ -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 diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c --- grub2.orig/kern/dl.c 2007-07-22 01:32:26.000000000 +0200 +++ grub2/kern/dl.c 2007-11-11 18:01:50.578125000 +0100 @@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym; #endif +#ifdef HAVE_ASM_USCORE +# define SYM_USCORE "_" +#else +# define SYM_USCORE "" +#endif + \f 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE) 2007-11-11 18:24 ` Christian Franke @ 2007-11-18 6:18 ` Robert Millan 2007-11-18 20:52 ` Christian Franke 0 siblings, 1 reply; 5+ messages in thread From: Robert Millan @ 2007-11-18 6:18 UTC (permalink / raw) To: The development of GRUB 2 On Sun, Nov 11, 2007 at 07:24:52PM +0100, Christian Franke wrote: > > 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" > diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c > --- grub2.orig/kern/dl.c 2007-07-22 01:32:26.000000000 +0200 > +++ grub2/kern/dl.c 2007-11-11 18:01:50.578125000 +0100 > @@ -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? -- 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 /.) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE) 2007-11-18 6:18 ` Robert Millan @ 2007-11-18 20:52 ` Christian Franke 0 siblings, 0 replies; 5+ messages in thread From: Christian Franke @ 2007-11-18 20:52 UTC (permalink / raw) To: The development of GRUB 2 Robert Millan wrote: > >> diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c >> --- grub2.orig/kern/dl.c 2007-07-22 01:32:26.000000000 +0200 >> +++ grub2/kern/dl.c 2007-11-11 18:01:50.578125000 +0100 >> @@ -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? > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-11-18 20:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-11-10 19:03 [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE) Christian Franke 2007-11-10 21:24 ` Robert Millan 2007-11-11 18:24 ` Christian Franke 2007-11-18 6:18 ` Robert Millan 2007-11-18 20:52 ` Christian Franke
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.