PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

Here is the re-submission of

http://sourceware.org/ml/binutils/2011-04/msg00063.html

OK to install?

Thanks.

H.J.
---
bfd/

2011-04-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/12639
	* elfcode.h (elf_slurp_symbol_table): Check valid local symbols.

binutils/

2011-04-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/12639
	* readelf.c (process_symbol_table): Detect corrupted symtab.

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 28b6b90..c56b4f1 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1218,6 +1218,9 @@ elf_slurp_symbol_table (bfd *abfd, asymbol **symptrs, bfd_boolean dynamic)
     sym = symbase = NULL;
   else
     {
+      /* Start of global symbols */
+      Elf_Internal_Sym *start_global;
+
       isymbuf = bfd_elf_get_elf_syms (abfd, hdr, symcount, 0,
 				      NULL, NULL, NULL);
       if (isymbuf == NULL)
@@ -1262,6 +1265,9 @@ elf_slurp_symbol_table (bfd *abfd, asymbol **symptrs, bfd_boolean dynamic)
       if (xver != NULL)
 	++xver;
       isymend = isymbuf + symcount;
+      start_global = isymbuf;
+      if (!elf_bad_symtab (abfd))
+	start_global += hdr->sh_info;
       for (isym = isymbuf + 1, sym = symbase; isym < isymend; isym++, sym++)
 	{
 	  memcpy (&sym->internal_elf_sym, isym, sizeof (Elf_Internal_Sym));
@@ -1306,6 +1312,18 @@ elf_slurp_symbol_table (bfd *abfd, asymbol **symptrs, bfd_boolean dynamic)
 	  if ((abfd->flags & (EXEC_P | DYNAMIC)) != 0)
 	    sym->symbol.value -= sym->symbol.section->vma;
 
+	  if (isym < start_global
+	      && ELF_ST_BIND (isym->st_info) != STB_LOCAL)
+	    {
+	      (*_bfd_error_handler)
+		(_("%s: corrupted local symbol `%s'"),
+		 abfd->filename, sym->symbol.name);
+
+	      /* Force it to local symbol.  */
+	      isym->st_info = ELF_ST_INFO (STB_LOCAL,
+					   ELF_ST_TYPE (isym->st_info));
+	    }
+
 	  switch (ELF_ST_BIND (isym->st_info))
 	    {
 	    case STB_LOCAL:
diff --git a/binutils/readelf.c b/binutils/readelf.c
index f8c6bb3..ebeddb7 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -8825,7 +8825,7 @@ process_symbol_table (FILE * file)
 	   i < elf_header.e_shnum;
 	   i++, section++)
 	{
-	  unsigned int si;
+	  unsigned int si, start_global;
 	  char * strtab = NULL;
 	  unsigned long int strtab_size = 0;
 	  Elf_Internal_Sym * symtab;
@@ -8857,6 +8857,7 @@ process_symbol_table (FILE * file)
 	  if (symtab == NULL)
 	    continue;
 
+	  start_global = section->sh_info;
 	  if (section->sh_link == elf_header.e_shstrndx)
 	    {
 	      strtab = string_table;
@@ -8883,7 +8884,11 @@ process_symbol_table (FILE * file)
 	      putchar (' ');
 	      print_vma (psym->st_size, DEC_5);
 	      printf (" %-7s", get_symbol_type (ELF_ST_TYPE (psym->st_info)));
-	      printf (" %-6s", get_symbol_binding (ELF_ST_BIND (psym->st_info)));
+	      if (si < start_global
+		  && ELF_ST_BIND (psym->st_info) != STB_LOCAL)
+		printf (" %-6s", "<corrupt>");
+	      else
+		printf (" %-6s", get_symbol_binding (ELF_ST_BIND (psym->st_info)));
 	      printf (" %-7s", get_symbol_visibility (ELF_ST_VISIBILITY (psym->st_other)));
 	      /* Check to see if any other bits in the st_other field are set.
 	         Note - displaying this information disrupts the layout of the

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[Alan Modra]

On Thu, Jul 05, 2012 at 11:21:56AM -0700, H.J. Lu wrote:
> Here is the re-submission of
> 
> http://sourceware.org/ml/binutils/2011-04/msg00063.html

And here is the review, again.  I still don't like it.

http://sourceware.org/ml/binutils/2011-04/msg00088.html

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

On Thu, Apr 7, 2011 at 2:31 AM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Apr 06, 2011 at 07:47:31AM -0700, H.J. Lu wrote:
>> On Wed, Apr 6, 2011 at 7:43 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> > On Tue, Apr 5, 2011 at 11:58 PM, Alan Modra <amodra@gmail.com> wrote:
>> >> On the readelf patch, I'd be inclined to report "Bad symtab" at the
>> >> end of the symbol table and not special case mips.
>> >
>> > readelf displays many "<corrupt>" without something like
>> > "Bad XXXX" at the end.  I don't think symtab is that special.
>> >
>
> What I should have said here is:  Don't special case mips.  Display the
> symbol binding as you normally do, ie. no <corrupt> message, and
> report "Bad symtab" at the end of the table.
>
> The idea is that "Bad symtab" will be displayed for your testcase, and
> for targets that set elf_bad_symtab.  I'm trying to make the readelf
> change more generally useful.
>
> After all, reporting the symbol binding <corrupt> isn't really true.
> If anything is corrupt, it's the symtab sh_info in your testcase.
>

People will see normal output and "Bad symtab" at the end
without any clue for what went wrong.

-- 
H.J.

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[Alan Modra]

On Wed, Apr 06, 2011 at 07:47:31AM -0700, H.J. Lu wrote:
> On Wed, Apr 6, 2011 at 7:43 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Tue, Apr 5, 2011 at 11:58 PM, Alan Modra <amodra@gmail.com> wrote:
> >> On the readelf patch, I'd be inclined to report "Bad symtab" at the
> >> end of the symbol table and not special case mips.
> >
> > readelf displays many "<corrupt>" without something like
> > "Bad XXXX" at the end.  I don't think symtab is that special.
> >

What I should have said here is:  Don't special case mips.  Display the
symbol binding as you normally do, ie. no <corrupt> message, and
report "Bad symtab" at the end of the table.

The idea is that "Bad symtab" will be displayed for your testcase, and
for targets that set elf_bad_symtab.  I'm trying to make the readelf
change more generally useful.

After all, reporting the symbol binding <corrupt> isn't really true.
If anything is corrupt, it's the symtab sh_info in your testcase.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

On Wed, Apr 6, 2011 at 7:43 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Apr 5, 2011 at 11:58 PM, Alan Modra <amodra@gmail.com> wrote:
>> On Tue, Apr 05, 2011 at 11:02:06PM -0700, H.J. Lu wrote:
>>> d) nm/readelf reports error on those bad input files.
>>>
>>> That is what my patch implements.
>>
>> Not exactly.  Your elfcode.h:elf_slurp_symbol_table patch issues a
>> message but doesn't return an error.  I think you ought to call
>> bfd_set_error and exit the function via error_return.
>
> Then we got
>
> [hjl@gnu-6 cq167859]$ make
> ./ld -o bad tr70098.bad.o start.o
> start.o: In function `_start':
> (.text+0x1): undefined reference to `main'
> make: *** [bad] Error 1
> [hjl@gnu-6 cq167859]$ ./nm tr70098.bad.o
> BFD: tr70098.bad.o: corrupted local symbol `main'
> ./nm: tr70098.bad.o: No symbols
> [hjl@gnu-6 cq167859]$
>
> "nm" won't display any symbols. I don't think it is very helpful.
>
>> On the readelf patch, I'd be inclined to report "Bad symtab" at the
>> end of the symbol table and not special case mips.
>>
>
> readelf displays many "<corrupt>" without something like
> "Bad XXXX" at the end.  I don't think symtab is that special.
>

Here is the updated patch without special case for MIPS.
OK to install?

Thanks.

-- 
H.J.
---
bfd/

2011-04-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/12639
	* elfcode.h (elf_slurp_symbol_table): Check valid local symbols.

binutils/

2011-04-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/12639
	* readelf.c (process_symbol_table): Detect corrupted symtab.

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 28b6b90..c56b4f1 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1218,6 +1218,9 @@ elf_slurp_symbol_table (bfd *abfd, asymbol
**symptrs, bfd_boolean dynamic)
     sym = symbase = NULL;
   else
     {
+      /* Start of global symbols */
+      Elf_Internal_Sym *start_global;
+
       isymbuf = bfd_elf_get_elf_syms (abfd, hdr, symcount, 0,
 				      NULL, NULL, NULL);
       if (isymbuf == NULL)
@@ -1262,6 +1265,9 @@ elf_slurp_symbol_table (bfd *abfd, asymbol
**symptrs, bfd_boolean dynamic)
       if (xver != NULL)
 	++xver;
       isymend = isymbuf + symcount;
+      start_global = isymbuf;
+      if (!elf_bad_symtab (abfd))
+	start_global += hdr->sh_info;
       for (isym = isymbuf + 1, sym = symbase; isym < isymend; isym++, sym++)
 	{
 	  memcpy (&sym->internal_elf_sym, isym, sizeof (Elf_Internal_Sym));
@@ -1306,6 +1312,18 @@ elf_slurp_symbol_table (bfd *abfd, asymbol
**symptrs, bfd_boolean dynamic)
 	  if ((abfd->flags & (EXEC_P | DYNAMIC)) != 0)
 	    sym->symbol.value -= sym->symbol.section->vma;

+	  if (isym < start_global
+	      && ELF_ST_BIND (isym->st_info) != STB_LOCAL)
+	    {
+	      (*_bfd_error_handler)
+		(_("%s: corrupted local symbol `%s'"),
+		 abfd->filename, sym->symbol.name);
+
+	      /* Force it to local symbol.  */
+	      isym->st_info = ELF_ST_INFO (STB_LOCAL,
+					   ELF_ST_TYPE (isym->st_info));
+	    }
+
 	  switch (ELF_ST_BIND (isym->st_info))
 	    {
 	    case STB_LOCAL:
diff --git a/binutils/readelf.c b/binutils/readelf.c
index f8c6bb3..ebeddb7 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -8825,7 +8825,7 @@ process_symbol_table (FILE * file)
 	   i < elf_header.e_shnum;
 	   i++, section++)
 	{
-	  unsigned int si;
+	  unsigned int si, start_global;
 	  char * strtab = NULL;
 	  unsigned long int strtab_size = 0;
 	  Elf_Internal_Sym * symtab;
@@ -8857,6 +8857,7 @@ process_symbol_table (FILE * file)
 	  if (symtab == NULL)
 	    continue;

+	  start_global = section->sh_info;
 	  if (section->sh_link == elf_header.e_shstrndx)
 	    {
 	      strtab = string_table;
@@ -8883,7 +8884,11 @@ process_symbol_table (FILE * file)
 	      putchar (' ');
 	      print_vma (psym->st_size, DEC_5);
 	      printf (" %-7s", get_symbol_type (ELF_ST_TYPE (psym->st_info)));
-	      printf (" %-6s", get_symbol_binding (ELF_ST_BIND (psym->st_info)));
+	      if (si < start_global
+		  && ELF_ST_BIND (psym->st_info) != STB_LOCAL)
+		printf (" %-6s", "<corrupt>");
+	      else
+		printf (" %-6s", get_symbol_binding (ELF_ST_BIND (psym->st_info)));
 	      printf (" %-7s", get_symbol_visibility (ELF_ST_VISIBILITY
(psym->st_other)));
 	      /* Check to see if any other bits in the st_other field are set.
 	         Note - displaying this information disrupts the layout of the

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

On Tue, Apr 5, 2011 at 11:58 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Apr 05, 2011 at 11:02:06PM -0700, H.J. Lu wrote:
>> d) nm/readelf reports error on those bad input files.
>>
>> That is what my patch implements.
>
> Not exactly.  Your elfcode.h:elf_slurp_symbol_table patch issues a
> message but doesn't return an error.  I think you ought to call
> bfd_set_error and exit the function via error_return.

Then we got

[hjl@gnu-6 cq167859]$ make
./ld -o bad tr70098.bad.o start.o
start.o: In function `_start':
(.text+0x1): undefined reference to `main'
make: *** [bad] Error 1
[hjl@gnu-6 cq167859]$ ./nm tr70098.bad.o
BFD: tr70098.bad.o: corrupted local symbol `main'
./nm: tr70098.bad.o: No symbols
[hjl@gnu-6 cq167859]$

"nm" won't display any symbols. I don't think it is very helpful.

> On the readelf patch, I'd be inclined to report "Bad symtab" at the
> end of the symbol table and not special case mips.
>

readelf displays many "<corrupt>" without something like
"Bad XXXX" at the end.  I don't think symtab is that special.

-- 
H.J.

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[Alan Modra]

On Tue, Apr 05, 2011 at 11:02:06PM -0700, H.J. Lu wrote:
> d) nm/readelf reports error on those bad input files.
> 
> That is what my patch implements.

Not exactly.  Your elfcode.h:elf_slurp_symbol_table patch issues a
message but doesn't return an error.  I think you ought to call
bfd_set_error and exit the function via error_return.

On the readelf patch, I'd be inclined to report "Bad symtab" at the
end of the symbol table and not special case mips.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

On Tue, Apr 5, 2011 at 10:01 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Apr 05, 2011 at 08:13:48PM -0700, H.J. Lu wrote:
>> Linker will treat main as local no matter what symtab says.
>
> Right.
>
>> For all practical purposes, any symbol before sh_info is
>> a local symbol.
>>
>> > What created this bad object file?
>>
>> They should remain nameless if possible :-).
>
> :-)  I think you have three options:
> a) Issue an error, and refuse to link the bad object file, or

We do get a linker error today:

[hjl@gnu-6 cq167859]$ make
./ld -o bad tr70098.bad.o start.o
start.o: In function `_start':
(.text+0x1): undefined reference to `main'
make: *** [bad] Error 1
[hjl@gnu-6 cq167859]$

since main is treated as local. But the output of "nm/readelf" looks normal.
My change makes "nm/readelf" to flag the bad symtab.

> b) Create a new target with elf_bad_symtab set for the target, or
> c) Provide some other means of setting elf_bad_symtab.
>
> I really don't like (b).  (c) is a pain, since it isn't just ld, but
> objcopy, objdump, etc.
>

I don't want (b) nor (c). I want

d) nm/readelf reports error on those bad input files.

That is what my patch implements.


-- 
H.J.

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[Alan Modra]

On Tue, Apr 05, 2011 at 08:13:48PM -0700, H.J. Lu wrote:
> Linker will treat main as local no matter what symtab says.

Right.

> For all practical purposes, any symbol before sh_info is
> a local symbol.
> 
> > What created this bad object file?
> 
> They should remain nameless if possible :-).

:-)  I think you have three options:
a) Issue an error, and refuse to link the bad object file, or
b) Create a new target with elf_bad_symtab set for the target, or
c) Provide some other means of setting elf_bad_symtab.

I really don't like (b).  (c) is a pain, since it isn't just ld, but
objcopy, objdump, etc.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

On Tue, Apr 5, 2011 at 7:51 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Apr 05, 2011 at 10:08:49AM -0700, H.J. Lu wrote:
>> +           /* Force it to local symbol.  */
>> +           isym->st_info = ELF_ST_INFO (STB_LOCAL,
>> +                                        ELF_ST_TYPE (isym->st_info));
>> +         }
>> +
>
> I think this is wrong.  The testcase clearly wants "main" as a global.

That is the problem.  elflink.c has:

  /* The sh_info field of the symtab header tells us where the
     external symbols start.  We don't care about the local symbols at
     this point.  */
  if (elf_bad_symtab (abfd))
    {
      extsymcount = symcount;
      extsymoff = 0;
    }
  else
    {
      extsymcount = symcount - hdr->sh_info;
      extsymoff = hdr->sh_info;
    }

Linker will treat main as local no matter what symtab says.
For all practical purposes, any symbol before sh_info is
a local symbol.

> What created this bad object file?

They should remain nameless if possible :-).


-- 
H.J.

Re: PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[Alan Modra]

On Tue, Apr 05, 2011 at 10:08:49AM -0700, H.J. Lu wrote:
> +	      /* Force it to local symbol.  */
> +	      isym->st_info = ELF_ST_INFO (STB_LOCAL,
> +					   ELF_ST_TYPE (isym->st_info));
> +	    }
> +

I think this is wrong.  The testcase clearly wants "main" as a global.
What created this bad object file?

-- 
Alan Modra
Australia Development Lab, IBM

PATCH: PR binutils/12639: nm/readelf failed to detect corrupted symtab

[H.J. Lu]

Hi,

This patch detects corrupted symtab.  OK to install?

Thanks.


H.J.
---
bfd/

2011-04-05  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/12639
	* elfcode.h (elf_slurp_symbol_table): Check valid local symbols.

binutils/

2011-04-05  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/12639
	* readelf.c (process_symbol_table): Detect corrupted symtab.

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 28b6b90..c56b4f1 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1218,6 +1218,9 @@ elf_slurp_symbol_table (bfd *abfd, asymbol **symptrs, bfd_boolean dynamic)
     sym = symbase = NULL;
   else
     {
+      /* Start of global symbols */
+      Elf_Internal_Sym *start_global;
+
       isymbuf = bfd_elf_get_elf_syms (abfd, hdr, symcount, 0,
 				      NULL, NULL, NULL);
       if (isymbuf == NULL)
@@ -1262,6 +1265,9 @@ elf_slurp_symbol_table (bfd *abfd, asymbol **symptrs, bfd_boolean dynamic)
       if (xver != NULL)
 	++xver;
       isymend = isymbuf + symcount;
+      start_global = isymbuf;
+      if (!elf_bad_symtab (abfd))
+	start_global += hdr->sh_info;
       for (isym = isymbuf + 1, sym = symbase; isym < isymend; isym++, sym++)
 	{
 	  memcpy (&sym->internal_elf_sym, isym, sizeof (Elf_Internal_Sym));
@@ -1306,6 +1312,18 @@ elf_slurp_symbol_table (bfd *abfd, asymbol **symptrs, bfd_boolean dynamic)
 	  if ((abfd->flags & (EXEC_P | DYNAMIC)) != 0)
 	    sym->symbol.value -= sym->symbol.section->vma;
 
+	  if (isym < start_global
+	      && ELF_ST_BIND (isym->st_info) != STB_LOCAL)
+	    {
+	      (*_bfd_error_handler)
+		(_("%s: corrupted local symbol `%s'"),
+		 abfd->filename, sym->symbol.name);
+
+	      /* Force it to local symbol.  */
+	      isym->st_info = ELF_ST_INFO (STB_LOCAL,
+					   ELF_ST_TYPE (isym->st_info));
+	    }
+
 	  switch (ELF_ST_BIND (isym->st_info))
 	    {
 	    case STB_LOCAL:
diff --git a/binutils/readelf.c b/binutils/readelf.c
index f8c6bb3..fdfc403 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -8825,7 +8825,7 @@ process_symbol_table (FILE * file)
 	   i < elf_header.e_shnum;
 	   i++, section++)
 	{
-	  unsigned int si;
+	  unsigned int si, start_global;
 	  char * strtab = NULL;
 	  unsigned long int strtab_size = 0;
 	  Elf_Internal_Sym * symtab;
@@ -8857,6 +8857,11 @@ process_symbol_table (FILE * file)
 	  if (symtab == NULL)
 	    continue;
 
+	  /* FIXME: Symbol tables in IRIX 5 and 6 are broken.  */
+	  if (elf_header.e_machine == EM_MIPS)
+	    start_global = 0;
+	  else
+	    start_global = section->sh_info;
 	  if (section->sh_link == elf_header.e_shstrndx)
 	    {
 	      strtab = string_table;
@@ -8883,7 +8888,11 @@ process_symbol_table (FILE * file)
 	      putchar (' ');
 	      print_vma (psym->st_size, DEC_5);
 	      printf (" %-7s", get_symbol_type (ELF_ST_TYPE (psym->st_info)));
-	      printf (" %-6s", get_symbol_binding (ELF_ST_BIND (psym->st_info)));
+	      if (si < start_global
+		  && ELF_ST_BIND (psym->st_info) != STB_LOCAL)
+		printf (" %-6s", "<corrupt>");
+	      else
+		printf (" %-6s", get_symbol_binding (ELF_ST_BIND (psym->st_info)));
 	      printf (" %-7s", get_symbol_visibility (ELF_ST_VISIBILITY (psym->st_other)));
 	      /* Check to see if any other bits in the st_other field are set.
 	         Note - displaying this information disrupts the layout of the