Re: patch for readelf - dumping NOTE sections

Re: patch for readelf - dumping NOTE sections

[Nick Clifton]

Hi Fred,

: + 1999-11-24  Fred Fish  <fnf@cygnus.com>
: + 
: + 	* readelf.c (process_note): Change arg from Elf_External_Note
: + 	to Elf32_Internal_Note, which also turns the function body
: + 	into little more than a call to printf.
: + 	(process_corefile_note_segment):  Substantially rewritten
: + 	to properly handle case where target and host are different
: + 	endianness, handle note sections with padding, and add some
: + 	cruft to handle notes with unterminated name data.
: + 

Thanks for this patch.  I do however have a few comments:

  * The normal convention is just submit the ChangeLog entry as
    straight text, not a context diff.  This is because ChangeLogs
    change so rapidly that it is often impossible to apply a ChangeLog
    patch automatically.

  * Your patch contained a potential memory leak, in that if the
    malloc of temp failed the code would return without freeing
    pnotes.  I have fixed this.

  * The was also a potential memory corruption bug in that you used
    strcpy() to copy the un-null-terminated name string into temp.
    This could easily extend over the end of the memory allocated for
    temp.  I have fixed this by using strncpy() to copy the name and
    then forcing the last byte of the array to zero.

Apart from that the patch was fine and I have gone ahead and checked
it in.

Cheers
	Nick

Re: patch for readelf - dumping NOTE sections

[fnf]

>   * The normal convention is just submit the ChangeLog entry as
>     straight text, not a context diff.

Yup, I neglected to do that, even though I already knew that.

>   * Your patch contained a potential memory leak ...
>   * The was also a potential memory corruption bug ...

Thanks.  Those were partly due to my rushed fix once I discovered the
problem with unterminated names on Linux.  I should have taken more
time to review that last minute change.

Sorry about the sloppy work.  :-(

-Fred

patch for readelf - dumping NOTE sections

[fnf]

While attempting to dump a corefile created on a big endian machine
using a copy of readelf compiled on a little endian host, I ran into a
problem where readelf would try to malloc a huge chunk of memory, and
when that failed, eventually coredump.

I believe this patch fixes the endianness problem, as well as a couple
other minor nits I noticed.  One of the nits is that the namesz and
descsz fields do not include the size of any padding needed to align
the note descriptors and also align the next note entry.

While investigating this problem, I discovered that core files
generated on Linux appear to violate the ELF standard that I have, by
not properly null terminating the name field and including the null
byte in the namesz field.  This patch still accomodates that, but the
warning to diagnose the problem is currently commented out.

-Fred

Index: ChangeLog
===================================================================
RCS file: /cvs/binutils/binutils/binutils/ChangeLog,v
retrieving revision 1.89
diff -c -r1.89 ChangeLog
*** ChangeLog	1999/11/22 09:42:42	1.89
--- ChangeLog	1999/11/24 22:50:08
***************
*** 1,3 ****
--- 1,13 ----
+ 1999-11-24  Fred Fish  <fnf@cygnus.com>
+ 
+ 	* readelf.c (process_note): Change arg from Elf_External_Note
+ 	to Elf32_Internal_Note, which also turns the function body
+ 	into little more than a call to printf.
+ 	(process_corefile_note_segment):  Substantially rewritten
+ 	to properly handle case where target and host are different
+ 	endianness, handle note sections with padding, and add some
+ 	cruft to handle notes with unterminated name data.
+ 
  1999-11-22  Nick Clifton  <nickc@cygnus.com>
  
  	* objcopy.c (copy_usage): Reformat.
Index: readelf.c
===================================================================
RCS file: /cvs/binutils/binutils/binutils/readelf.c,v
retrieving revision 1.28
diff -c -r1.28 readelf.c
*** readelf.c	1999/10/04 18:59:13	1.28
--- readelf.c	1999/11/24 22:50:59
***************
*** 200,206 ****
  static const char *       get_osabi_name              PARAMS ((unsigned char));
  static int		  guess_is_rela               PARAMS ((unsigned long));
  static char * 		  get_note_type		         PARAMS ((unsigned int));
! static int		  process_note		         PARAMS ((Elf_External_Note *));
  static int		  process_corefile_note_segment  PARAMS ((FILE *, unsigned long, unsigned long));
  static int		  process_corefile_note_segments PARAMS ((FILE *));
  static int 		  process_corefile_contents	 PARAMS ((FILE *));
--- 200,206 ----
  static const char *       get_osabi_name              PARAMS ((unsigned char));
  static int		  guess_is_rela               PARAMS ((unsigned long));
  static char * 		  get_note_type		         PARAMS ((unsigned int));
! static int		  process_note		         PARAMS ((Elf32_Internal_Note *));
  static int		  process_corefile_note_segment  PARAMS ((FILE *, unsigned long, unsigned long));
  static int		  process_corefile_note_segments PARAMS ((FILE *));
  static int 		  process_corefile_contents	 PARAMS ((FILE *));
***************
*** 6605,6634 ****
      }
  }
  
  static int
  process_note (pnote)
!   Elf_External_Note * pnote;
  {
-   Elf32_Internal_Note * internal;
-   char * pname;
- 
-   internal = (Elf32_Internal_Note *) pnote;
-   pname = malloc (internal->namesz + 1);
- 
-   if (pname == NULL)
-     {
-       error (_("Out of memory\n"));
-       return 0;
-     }
- 
-   memcpy (pname, pnote->name, internal->namesz);
-   pname[internal->namesz] = '\0';
- 
    printf ("  %s\t\t0x%08lx\t%s\n",
!   	  pname, internal->descsz, get_note_type (internal->type));
! 
!   free (pname);
! 
    return 1;
  }
  
--- 6605,6623 ----
      }
  }
  
+ /* Note that by the ELF standard, the name field is already null byte
+    terminated, and namesz includes the terminating null byte.
+    I.E. the value of namesz for the name "FSF" is 4.
+ 
+    If the value of namesz is zero, there is no name present. */
+ 
  static int
  process_note (pnote)
!   Elf32_Internal_Note * pnote;
  {
    printf ("  %s\t\t0x%08lx\t%s\n",
! 	  pnote->namesz ? pnote -> namedata : "(NONE)",
!   	  pnote->descsz, get_note_type (pnote->type));
    return 1;
  }
  
***************
*** 6640,6672 ****
  {
    Elf_External_Note *  pnotes;
    Elf_External_Note *  external;
-   Elf32_Internal_Note* internal;
-   unsigned int	       notesz;
-   unsigned int         nlength;
-   unsigned char *      p;
    int                  res = 1;
  
    if (length <= 0)
      return 0;
  
    GET_DATA_ALLOC (offset, length, pnotes, Elf_External_Note *, "notes");
- 
    external = pnotes;
-   p = (unsigned char *) pnotes;
-   nlength = length;
  
    printf (_("\nNotes at offset 0x%08lx with length 0x%08lx:\n"), offset, length);
    printf (_("  Owner\t\tData size\tDescription\n"));
  
!   while (nlength > 0)
      {
!       res &= process_note (external);
  
!       internal = (Elf32_Internal_Note *) p;
!       notesz   = 3 * sizeof(unsigned long) + internal->namesz + internal->descsz;
!       nlength -= notesz;
!       p       += notesz;
!       external = (Elf_External_Note *) p;
      }
  
    free (pnotes);
--- 6629,6682 ----
  {
    Elf_External_Note *  pnotes;
    Elf_External_Note *  external;
    int                  res = 1;
  
    if (length <= 0)
      return 0;
  
    GET_DATA_ALLOC (offset, length, pnotes, Elf_External_Note *, "notes");
    external = pnotes;
  
    printf (_("\nNotes at offset 0x%08lx with length 0x%08lx:\n"), offset, length);
    printf (_("  Owner\t\tData size\tDescription\n"));
  
!   while (external < ((char *) pnotes + length))
      {
!       Elf32_Internal_Note inote;
!       char *temp = NULL;
  
!       inote.type = BYTE_GET (external->type);
!       inote.namesz = BYTE_GET (external->namesz);
!       inote.namedata = external->name;
!       inote.descsz = BYTE_GET (external->descsz);
!       inote.descdata = inote.namedata + align_power (inote.namesz, 2);
!       inote.descpos = offset + (inote.descdata - (char *) pnotes);
!       external = inote.descdata + align_power (inote.descsz, 2);
! 
!       /* Verify that name is null terminated.  It appears that at least
! 	 one version of Linux (RedHat 6.0) generates corefiles that don't
! 	 comply with the ELF spec by failing to include the null byte in
! 	 namesz. */
!       if (inote.namedata[inote.namesz] != '\0')
! 	{
! 	  temp = malloc (inote.namesz + 1);
! 	  if (temp == NULL)
! 	    {
! 	      error (_("Out of memory\n"));
! 	      return (0);
! 	  }
! 	  strcpy (temp, inote.namedata);
! 	  /* warn (_("'%s' NOTE name not properly null terminated\n"), temp); */
! 	  inote.namedata = temp;
!       }
! 
!       res &= process_note (&inote);
! 
!       if (temp != NULL)
! 	{
! 	  free (temp);
! 	  temp = NULL;
! 	}
      }
  
    free (pnotes);