[6/5] Only warn about malformed win32pstatus notes

Message ID 20200721131550.26976-1-jon.turney@dronecode.org.uk
State New
Headers show
Series
  • Untitled series #28089
Related show

Commit Message

Jon Turney July 21, 2020, 1:15 p.m.
bfd/ChangeLog:

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

	* elf.c (elfcore_grok_win32pstatus): Warn on malformed
	win32pstatus notes, and return TRUE so we continue rather than
	stopping as if it was an error.
---
 bfd/ChangeLog |  6 ++++++
 bfd/elf.c     | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 32 insertions(+), 13 deletions(-)

-- 
2.27.0

Comments

Jon Turney July 31, 2020, 1:15 p.m. | #1
On 21/07/2020 14:15, Jon Turney wrote:
> bfd/ChangeLog:

> 

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

> 

> 	* elf.c (elfcore_grok_win32pstatus): Warn on malformed

> 	win32pstatus notes, and return TRUE so we continue rather than

> 	stopping as if it was an error.

> ---


Ping?
Jose E. Marchesi via Binutils Aug. 12, 2020, 10:34 a.m. | #2
Hi Jon,

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

>>

>>     * elf.c (elfcore_grok_win32pstatus): Warn on malformed

>>     win32pstatus notes, and return TRUE so we continue rather than

>>     stopping as if it was an error.

>> ---

> 

> Ping?


Oops - sorry - I have been off on PTO.  Patch approved - please apply.

Cheers
  Nick
Tom Tromey Aug. 12, 2020, 2:51 p.m. | #3
>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:


Jon> 	* elf.c (elfcore_grok_win32pstatus): Warn on malformed
Jon> 	win32pstatus notes, and return TRUE so we continue rather than
Jon> 	stopping as if it was an error.

On my machine (x86-64 Fedora 32), this patch causes:

../../binutils-gdb/bfd/elf.c: In function ‘elfcore_grok_win32pstatus’:
../../binutils-gdb/bfd/elf.c:10168:12: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
10168 |   if (type > (sizeof(size_check)/sizeof(size_check[0])))
      |            ^


Changing 'type' from int to 'unsigned' makes it compile for me and seems
to be correct.

Tom
Jon Turney Aug. 12, 2020, 3:39 p.m. | #4
On 12/08/2020 15:51, Tom Tromey wrote:
>>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:

> 

> Jon> 	* elf.c (elfcore_grok_win32pstatus): Warn on malformed

> Jon> 	win32pstatus notes, and return TRUE so we continue rather than

> Jon> 	stopping as if it was an error.

> 

> On my machine (x86-64 Fedora 32), this patch causes:

> 

> ../../binutils-gdb/bfd/elf.c: In function ‘elfcore_grok_win32pstatus’:

> ../../binutils-gdb/bfd/elf.c:10168:12: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]

> 10168 |   if (type > (sizeof(size_check)/sizeof(size_check[0])))

>        |            ^

> 

> 

> Changing 'type' from int to 'unsigned' makes it compile for me and seems

> to be correct.


Thanks.

I applied that change (patch attached) as trivial.
From 16af4196ecfd90f4c2693137524bc28426863064 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 12 Aug 2020 16:34:47 +0100
Subject: [PATCH] Fix signedness comparison warning in
 elfcore_grok_win32pstatus()

bfd/ChangeLog:

2020-08-12  Jon Turney  <jon.turney@dronecode.org.uk>

	* elf.c (elfcore_grok_win32pstatus): Use unsigned int for
	win32pstatus note type to avoid signedness comparison warning.
---
 bfd/ChangeLog | 5 +++++
 bfd/elf.c     | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/bfd/elf.c b/bfd/elf.c
index e8c457cd5d5..54c72043418 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -10142,7 +10142,7 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
   size_t len;
   size_t name_size;
   asection *sect;
-  int type;
+  unsigned int type;
   int is_active_thread;
   bfd_vma base_addr;
Jon Turney Aug. 22, 2020, 2:17 p.m. | #5
On 21/07/2020 14:15, Jon Turney wrote:
> bfd/ChangeLog:

> 

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

> 

> 	* elf.c (elfcore_grok_win32pstatus): Warn on malformed

> 	win32pstatus notes, and return TRUE so we continue rather than

> 	stopping as if it was an error.

> ---

>   bfd/ChangeLog |  6 ++++++

>   bfd/elf.c     | 39 ++++++++++++++++++++++++++-------------

>   2 files changed, 32 insertions(+), 13 deletions(-)

> 

> diff --git a/bfd/elf.c b/bfd/elf.c

> index b836f041724..f28b553b67b 100644

> --- a/bfd/elf.c

> +++ b/bfd/elf.c

[...]>         if (note->descsz < 12 + name_size)
> -        return FALSE;

> +        {

> +          _bfd_error_handler (_("%pB: win32pstatus NOTE_INFO_MODULE of size %lu is too small to contain a name of size %zu"),

> +                              abfd, note->descsz, name_size);

> +          return TRUE;

> +        }


As reported at [1], using '%zu' here is incorrect, as it's not handled 
by _bfd_error_handler().

I propose to apply the attached as obvious.

[1] https://sourceware.org/pipermail/gdb-patches/2020-August/171391.html
From c261718f3cebf280f9088433f34793428f9c455a Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Fri, 21 Aug 2020 16:30:00 +0100
Subject: [PATCH] Fix erroroneous use of '%zu' in elfcore_grok_win32pstatus

As reported in [1], _bfd_error_handler() doesn't support '%zu'.

module_name_size is always 32-bits in the data structure we are
extracting it from, so use an unsigned int to store it instead.

[1] https://sourceware.org/pipermail/gdb-patches/2020-August/171391.html

bfd/ChangeLog:

2020-08-21  Jon Turney  <jon.turney@dronecode.org.uk>

	* elf.c (elfcore_grok_win32pstatus): Change name_size to unsigned
	int. Use '%u' format with  _bfd_error_handler to render it.
---
 bfd/ChangeLog | 5 +++++
 bfd/elf.c     | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/bfd/elf.c b/bfd/elf.c
index ecd9217b34d..f32118ad404 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -10140,7 +10140,7 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
   char buf[30];
   char *name;
   size_t len;
-  size_t name_size;
+  unsigned int name_size;
   asection *sect;
   unsigned int type;
   int is_active_thread;
@@ -10248,7 +10248,7 @@ elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
 
       if (note->descsz < 12 + name_size)
         {
-          _bfd_error_handler (_("%pB: win32pstatus NOTE_INFO_MODULE of size %lu is too small to contain a name of size %zu"),
+          _bfd_error_handler (_("%pB: win32pstatus NOTE_INFO_MODULE of size %lu is too small to contain a name of size %u"),
                               abfd, note->descsz, name_size);
           return TRUE;
         }

Patch

diff --git a/bfd/elf.c b/bfd/elf.c
index b836f041724..f28b553b67b 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -10154,21 +10154,36 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
 
   type = bfd_get_32 (abfd, note->descdata);
 
+  struct {
+    const char *type_name;
+    unsigned long min_size;
+  } size_check[] =
+      {
+       { "NOTE_INFO_PROCESS", 12 },
+       { "NOTE_INFO_THREAD", 12 },
+       { "NOTE_INFO_MODULE", 12 },
+       { "NOTE_INFO_MODULE64", 16 },
+      };
+
+  if (type > (sizeof(size_check)/sizeof(size_check[0])))
+      return TRUE;
+
+  if (note->descsz < size_check[type - 1].min_size)
+    {
+      _bfd_error_handler (_("%pB: warning: win32pstatus %s of size %lu bytes is too small"),
+                          abfd, size_check[type - 1].type_name, note->descsz);
+      return TRUE;
+    }
+
   switch (type)
     {
     case NOTE_INFO_PROCESS:
-      if (note->descsz < 12)
-        return FALSE;
-
       /* 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 FALSE;
-
       /* Make a ".reg/<tid>" section containing the Win32 API thread CONTEXT
          structure. */
       /* thread_info.tid */
@@ -10204,9 +10219,6 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
       /* Make a ".module/xxxxxxxx" section.  */
       if (type == NOTE_INFO_MODULE)
         {
-          if (note->descsz < 12)
-            return FALSE;
-
           /* module_info.base_address */
           base_addr = bfd_get_32 (abfd, note->descdata + 4);
           sprintf (buf, ".module/%08lx", (unsigned long) base_addr);
@@ -10215,9 +10227,6 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
         }
       else /* NOTE_INFO_MODULE64 */
         {
-          if (note->descsz < 16)
-            return FALSE;
-
           /* module_info.base_address */
           base_addr = bfd_get_64 (abfd, note->descdata + 4);
           sprintf (buf, ".module/%016lx", (unsigned long) base_addr);
@@ -10238,7 +10247,11 @@  elfcore_grok_win32pstatus (bfd *abfd, Elf_Internal_Note *note)
 	return FALSE;
 
       if (note->descsz < 12 + name_size)
-        return FALSE;
+        {
+          _bfd_error_handler (_("%pB: win32pstatus NOTE_INFO_MODULE of size %lu is too small to contain a name of size %zu"),
+                              abfd, note->descsz, name_size);
+          return TRUE;
+        }
 
       sect->size = note->descsz;
       sect->filepos = note->descpos;