[4/5] Refine size constraints applied to win32pstatus ELF notes

Message ID 20200712125542.1322-5-jon.turney@dronecode.org.uk
State Superseded
Headers show
Series
  • bfd: Add support for Cygwin x86_64 core dumps (v2)
Related show

Commit Message

Jon Turney July 12, 2020, 12:55 p.m.
Don't reject any win32pstatus notes smaller than minimum size for a
NOTE_INFO_THREAD.

This only happens to work because the Cygwin dumper tool currently
writes all these notes as the largest size of the union, (which wastes
lots of space in the core dump).

Instead, apply the appropriate size constraint for each win32pstatus
note type.

bfd/ChangeLog:

2020-07-11  Jon Turney  <jon.turney@dronecode.org.uk>

	* elf.c (elfcore_grok_win32pstatus): Don't apply size constraint
	for NOTE_INFO_THREAD to all win32pstatus ELF notes, instead apply
	appropriate size constraint for each win32pstatus note type.
---
 bfd/ChangeLog |  6 ++++++
 bfd/elf.c     | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.27.0

Comments

Jose E. Marchesi via Binutils July 13, 2020, 3:04 p.m. | #1
Hi Jon,

>      case NOTE_INFO_PROCESS:

> +      if (note->descsz < 12)

> +        return TRUE;

> +


Shouldn't this return, and the later ones, be "return FALSE" ?
After all you have a note type but insufficient space for valid note data.

Actually it looks to me like almost all of the returns in this function
should be "return FALSE" as there is something wrong with the note...

Cheers
  Nick
Jon Turney July 15, 2020, 1:22 p.m. | #2
On 13/07/2020 16:04, Nick Clifton wrote:
> Hi Jon,

> 

>>       case NOTE_INFO_PROCESS:

>> +      if (note->descsz < 12)

>> +        return TRUE;

>> +

> 

> Shouldn't this return, and the later ones, be "return FALSE" ?

> After all you have a note type but insufficient space for valid note data.


Yes, you are right.  Revised patches to follow.

> Actually it looks to me like almost all of the returns in this function

> should be "return FALSE" as there is something wrong with the note...


Perhaps the way this is written at the moment is a bit awkward as we 
don't really distinguish in the return code between (i) the contents of 
the note are malformed, and (ii) an internal error occurred while 
processing the note. Do we really want to stop with an error in both cases?
Jose E. Marchesi via Binutils July 15, 2020, 2:30 p.m. | #3
Hi Jon,

>> Actually it looks to me like almost all of the returns in this function

>> should be "return FALSE" as there is something wrong with the note...

> 

> Perhaps the way this is written at the moment is a bit awkward as we don't really distinguish in the return code between (i) the contents of the note are malformed, and (ii) an internal error occurred while processing the note. Do we really want to stop with an error in both cases?


Hmmm - generate an error/warning message of some kind - definitely.
Stop on the error - probably not.  After all these are core dumps
that we are dealing with and the user is going to want to get as 
much information out of them as they can.

Cheers
  Nick
Jon Turney July 21, 2020, 1:14 p.m. | #4
On 15/07/2020 15:30, Nick Clifton wrote:
> 

>>> Actually it looks to me like almost all of the returns in this function

>>> should be "return FALSE" as there is something wrong with the note...

>>

>> Perhaps the way this is written at the moment is a bit awkward as we don't really distinguish in the return code between (i) the contents of the note are malformed, and (ii) an internal error occurred while processing the note. Do we really want to stop with an error in both cases?

> 

> Hmmm - generate an error/warning message of some kind - definitely.

> Stop on the error - probably not.  After all these are core dumps

> that we are dealing with and the user is going to want to get as

> much information out of them as they can.


So, that seems to be back to 'return TRUE, but with a warning'.

I attempted to address that with the following patch [6/5]

Patch

diff --git a/bfd/elf.c b/bfd/elf.c
index 61a7f0930e2..b05d0d6c2db 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -10139,12 +10139,13 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
   char buf[30];
   char *name;
   size_t len;
+  size_t name_size;
   asection *sect;
   int type;
   int is_active_thread;
   bfd_vma base_addr;
 
-  if (note->descsz < 728)
+  if (note->descsz < 4)
     return TRUE;
 
   if (! CONST_STRNEQ (note->namedata, "win32"))
@@ -10155,12 +10156,18 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
   switch (type)
     {
     case NOTE_INFO_PROCESS:
+      if (note->descsz < 12)
+        return TRUE;
+
       /* FIXME: need to add ->core->command.  */
       elf_tdata (abfd)->core->pid = bfd_get_32 (abfd, note->descdata + 4);
       elf_tdata (abfd)->core->signal = bfd_get_32 (abfd, note->descdata + 8);
       break;
 
     case NOTE_INFO_THREAD:
+      if (note->descsz < 12)
+        return TRUE;
+
       /* Make a ".reg/<tid>" section containing the Win32 API thread CONTEXT
          structure. */
       /* thread_info.tid */
@@ -10192,6 +10199,9 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
       break;
 
     case NOTE_INFO_MODULE:
+      if (note->descsz < 12)
+        return TRUE;
+
       /* Make a ".module/xxxxxxxx" section.  */
       /* module_info.base_address */
       base_addr = bfd_get_32 (abfd, note->descdata + 4);
@@ -10209,6 +10219,11 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
       if (sect == NULL)
 	return FALSE;
 
+      /* module_info.module_name_size */
+      name_size = bfd_get_32 (abfd, note->descdata + 8);
+      if (note->descsz < 12 + name_size)
+        return TRUE;
+
       sect->size = note->descsz;
       sect->filepos = note->descpos;
       sect->alignment_power = 2;