[PATCH 0/8] Fix dumper for x86_64

[PATCH 0/8] Fix dumper for x86_64

[Jon Turney]

Jon Turney (8):
  Cygwin: Slightly improve error_start documentation
  Cygwin: Update ELF target used by dumper on x86_64
  Cygwin: Add a new win32_pstatus data type for modules on x86_64
  Cygwin: Make dumper scan more than first 4GB of VM on x86_64
  Cygwin: Fix bfd target for parsing PE files on x86_64 in dumper
  Cygwin: Fix dumper region order/overlap checking
  Cygwin: Handle excluded regions more robustly in dumper
  Cygwin: Consider DLL rebasing when computing dumper exclusions

 winsup/cygwin/include/cygwin/core_dump.h | 16 ++++++---
 winsup/doc/cygwinenv.xml                 |  6 +++-
 winsup/utils/dumper.cc                   | 23 ++++++++++---
 winsup/utils/dumper.h                    |  2 +-
 winsup/utils/parse_pe.cc                 | 43 +++++++++++++++++-------
 5 files changed, 66 insertions(+), 24 deletions(-)

-- 
2.27.0

[PATCH 1/8] Cygwin: Slightly improve error_start documentation

[Jon Turney]

---
 winsup/doc/cygwinenv.xml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/winsup/doc/cygwinenv.xml b/winsup/doc/cygwinenv.xml
index f549fee0d..a52b6ac19 100644
--- a/winsup/doc/cygwinenv.xml
+++ b/winsup/doc/cygwinenv.xml
@@ -28,9 +28,13 @@ Defaults to off.</para>
 which is useful for debugging.  <filename>Win32filepath</filename> is
 usually set to the path to <command>gdb</command> or
 <command>dumper</command>, for example
-<filename>C:\cygwin\bin\gdb.exe</filename>. 
+<filename>C:\cygwin\bin\gdb.exe</filename>.
 There is no default set.
 </para>
+<para>
+The filename of the executing program and it's Windows process id are appended
+to the command as arguments.
+</para>
 </listitem>
 
 <listitem>
-- 
2.27.0

[PATCH 2/8] Cygwin: Update ELF target used by dumper on x86_64

[Jon Turney]

Like [1], but actually making the effort to be 'usable' and 'tested'.

[1] https://cygwin.com/pipermail/cygwin/2019-October/242815.html
---
 winsup/utils/dumper.cc | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index 226c2283d..e16d80a36 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -645,7 +645,13 @@ dumper::init_core_dump ()
 {
   bfd_init ();
 
-  core_bfd = bfd_openw (file_name, "elf32-i386");
+#ifdef __x86_64__
+  const char *target = "elf64-x86-64";
+#else
+  const char *target = "elf32-i386";
+#endif
+
+  core_bfd = bfd_openw (file_name, target);
   if (core_bfd == NULL)
     {
       bfd_perror ("opening bfd");
@@ -658,7 +664,7 @@ dumper::init_core_dump ()
       goto failed;
     }
 
-  if (!bfd_set_arch_mach (core_bfd, bfd_arch_i386, 0))
+  if (!bfd_set_arch_mach (core_bfd, bfd_arch_i386, 0 /* = default */))
     {
       bfd_perror ("setting bfd architecture");
       goto failed;
-- 
2.27.0

[PATCH 3/8] Cygwin: Add a new win32_pstatus data type for modules on x86_64

[Jon Turney]

Take a bit more care with sizes in other data types to ensure they are
the same on x86 and x86_64.

Add some explanatory comments.
---
 winsup/cygwin/include/cygwin/core_dump.h | 16 ++++++++++++----
 winsup/utils/dumper.cc                   |  4 ++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/include/cygwin/core_dump.h b/winsup/cygwin/include/cygwin/core_dump.h
index 92ecae7ab..cd218ac45 100644
--- a/winsup/cygwin/include/cygwin/core_dump.h
+++ b/winsup/cygwin/include/cygwin/core_dump.h
@@ -11,17 +11,23 @@ details. */
 #ifndef _CYGWIN_CORE_DUMP_H
 #define _CYGWIN_CORE_DUMP_H
 
+/*
+  Note that elfcore_grok_win32pstatus() in libbfd relies on the precise layout
+  of these structures.
+*/
+
 #include <windows.h>
 
 #define	NOTE_INFO_PROCESS	1
 #define	NOTE_INFO_THREAD	2
 #define	NOTE_INFO_MODULE	3
+#define	NOTE_INFO_MODULE64	4
 
 struct win32_core_process_info
 {
   DWORD pid;
-  int signal;
-  int command_line_size;
+  DWORD signal;
+  DWORD command_line_size;
   char command_line[1];
 }
 #ifdef __GNUC__
@@ -40,10 +46,12 @@ struct win32_core_thread_info
 #endif
 ;
 
+/* Used with data_type NOTE_INFO_MODULE or NOTE_INFO_MODULE64, depending on
+   arch */
 struct win32_core_module_info
 {
   void* base_address;
-  int module_name_size;
+  DWORD module_name_size;
   char module_name[1];
 }
 #ifdef __GNUC__
@@ -53,7 +61,7 @@ struct win32_core_module_info
 
 struct win32_pstatus
 {
-  unsigned long data_type;
+  DWORD data_type;
   union
     {
       struct win32_core_process_info process_info;
diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index e16d80a36..dcf01e800 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -502,7 +502,11 @@ dumper::dump_module (asection * to, process_module * module)
   strncpy (header.elf_note_header.name, "win32module", NOTE_NAME_SIZE);
 #pragma GCC diagnostic pop
 
+#ifdef __x86_64__
+  module_pstatus_ptr->data_type = NOTE_INFO_MODULE64;
+#else
   module_pstatus_ptr->data_type = NOTE_INFO_MODULE;
+#endif
   module_pstatus_ptr->data.module_info.base_address = module->base_address;
   module_pstatus_ptr->data.module_info.module_name_size = strlen (module->name) + 1;
   strcpy (module_pstatus_ptr->data.module_info.module_name, module->name);
-- 
2.27.0

[PATCH 4/8] Cygwin: Make dumper scan more than first 4GB of VM on x86_64

[Jon Turney]

It's unclear that we need an end address here at all, or can just rely
on VirtualQueryEx() failing when we reach the end of memory regions.
---
 winsup/utils/dumper.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index dcf01e800..ccc4bd12f 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -296,7 +296,7 @@ dumper::collect_memory_sections ()
     return 0;
 
   LPBYTE current_page_address;
-  LPBYTE last_base = (LPBYTE) 0xFFFFFFFF;
+  LPBYTE last_base = (LPBYTE) -1;
   SIZE_T last_size = (SIZE_T) 0;
   SIZE_T done;
 
@@ -307,7 +307,7 @@ dumper::collect_memory_sections ()
   if (hProcess == NULL)
     return 0;
 
-  for (current_page_address = 0; current_page_address < (LPBYTE) 0xFFFF0000;)
+  for (current_page_address = 0; current_page_address < (LPBYTE) -1;)
     {
       if (!VirtualQueryEx (hProcess, current_page_address, &mbi, sizeof (mbi)))
 	break;
-- 
2.27.0

[PATCH 5/8] Cygwin: Fix bfd target for parsing PE files on x86_64 in dumper

[Jon Turney]

---
 winsup/utils/parse_pe.cc | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/winsup/utils/parse_pe.cc b/winsup/utils/parse_pe.cc
index 90b5c0b0d..d2a510a81 100644
--- a/winsup/utils/parse_pe.cc
+++ b/winsup/utils/parse_pe.cc
@@ -91,7 +91,14 @@ parse_pe (const char *file_name, exclusion * excl_list)
   if (file_name == NULL || excl_list == NULL)
     return 0;
 
-  bfd *abfd = bfd_openr (file_name, "pei-i386");
+#ifdef __x86_64__
+  const char *target = "pei-x86-64";
+#else
+  const char *target = "pei-i386";
+#endif
+
+  bfd *abfd = bfd_openr (file_name, target);
+
   if (abfd == NULL)
     {
       bfd_perror ("failed to open file");
-- 
2.27.0

[PATCH 6/8] Cygwin: Fix dumper region order/overlap checking

[Jon Turney]

---
 winsup/utils/parse_pe.cc | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/winsup/utils/parse_pe.cc b/winsup/utils/parse_pe.cc
index d2a510a81..653c46dfe 100644
--- a/winsup/utils/parse_pe.cc
+++ b/winsup/utils/parse_pe.cc
@@ -60,11 +60,9 @@ exclusion::sort_and_check ()
   for (process_mem_region * p = region; p < region + last - 1; p++)
     {
       process_mem_region *q = p + 1;
-      if (q == p + 1)
-	continue;
-      if (p->base + size > q->base)
+      if (p->base + p->size > q->base)
 	{
-	  fprintf (stderr, "region error @ (%p + %zd) > %p\n", p->base, size, q->base);
+	  fprintf (stderr, "region error @ (%p + 0x%0llx) > %p\n", p->base, p->size, q->base);
 	  return 0;
 	}
     }
-- 
2.27.0

[PATCH 7/8] Cygwin: Handle excluded regions more robustly in dumper

[Jon Turney]

Avoid trying to create regions with negative size if there's an error
(duplicate or out-of-order region) in the excluded region list.
---
 winsup/utils/dumper.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index ccc4bd12f..c0b3fd8ff 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -252,6 +252,9 @@ dumper::split_add_mem_region (LPBYTE base, SIZE_T size)
 	  continue;
 	}
 
+      if (p->base < last_base)
+	continue;
+
       add_mem_region (last_base, p->base - last_base);
       last_base = p->base + p->size;
     }
-- 
2.27.0

[PATCH 8/8] Cygwin: Consider DLL rebasing when computing dumper exclusions

[Jon Turney]

I think this would always have been neeeded, but is essential on x86_64,
as kernel32.dll has an ImageBase of 00000001:80000000 (but is always
rebased when loaded), which is identical to the cygwin DLL, causing
duplicate or overlapping exclusions.
---
 winsup/utils/dumper.cc   |  2 +-
 winsup/utils/dumper.h    |  2 +-
 winsup/utils/parse_pe.cc | 28 ++++++++++++++++++++--------
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index c0b3fd8ff..fa3fe1fbc 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -284,7 +284,7 @@ dumper::add_module (LPVOID base_address)
   new_entity->u.module.base_address = base_address;
   new_entity->u.module.name = module_name;
 
-  parse_pe (module_name, excl_list);
+  parse_pe (module_name, excl_list, base_address);
 
   deb_printf ("added module %p %s\n", base_address, module_name);
   return 1;
diff --git a/winsup/utils/dumper.h b/winsup/utils/dumper.h
index 9367587bf..9c4b1f092 100644
--- a/winsup/utils/dumper.h
+++ b/winsup/utils/dumper.h
@@ -133,7 +133,7 @@ extern int deb_printf ( const char* format, ... );
 
 extern char* psapi_get_module_name ( HANDLE hProcess, LPVOID BaseAddress );
 
-extern int parse_pe ( const char* file_name, exclusion* excl_list );
+extern int parse_pe ( const char* file_name, exclusion* excl_list, LPVOID base_address );
 
 extern BOOL verbose;
 
diff --git a/winsup/utils/parse_pe.cc b/winsup/utils/parse_pe.cc
index 653c46dfe..0256ada53 100644
--- a/winsup/utils/parse_pe.cc
+++ b/winsup/utils/parse_pe.cc
@@ -70,21 +70,21 @@ exclusion::sort_and_check ()
 }
 
 static void
-select_data_section (bfd * abfd, asection * sect, PTR obj)
+select_data_section (bfd * abfd, asection * sect, exclusion *excl_list,
+		     SSIZE_T adj)
 {
-  exclusion *excl_list = (exclusion *) obj;
-
   if ((sect->flags & (SEC_CODE | SEC_DEBUGGING)) &&
       sect->vma && bfd_get_section_size (sect))
     {
-      excl_list->add ((LPBYTE) sect->vma, (SIZE_T) bfd_get_section_size (sect));
-      deb_printf ("excluding section: %20s %08lx\n", sect->name,
-		  bfd_get_section_size (sect));
+      bfd_vma vma = sect->vma + adj;
+      excl_list->add ((LPBYTE) vma, (SIZE_T) bfd_get_section_size (sect));
+      deb_printf ("excluding section: %20s %016lx %016lx %08lx\n", sect->name,
+		  sect->vma, vma, bfd_get_section_size (sect));
     }
 }
 
 int
-parse_pe (const char *file_name, exclusion * excl_list)
+parse_pe (const char *file_name, exclusion * excl_list, LPVOID base_address)
 {
   if (file_name == NULL || excl_list == NULL)
     return 0;
@@ -104,7 +104,19 @@ parse_pe (const char *file_name, exclusion * excl_list)
     }
 
   bfd_check_format (abfd, bfd_object);
-  bfd_map_over_sections (abfd, &select_data_section, (PTR) excl_list);
+
+  /* Compute the relocation offset for this DLL.  Unfortunately, we have to
+     guess at ImageBase (one page before vma of first section), since bfd
+     doesn't let us get at backend-private data */
+  bfd_vma imagebase = abfd->sections->vma - 0x1000;
+  SSIZE_T adj = (bfd_vma)base_address - imagebase;
+  deb_printf("imagebase relocated from %016lx to %016lx (%016lx)\n",
+	     imagebase, base_address, adj);
+
+  asection *p;
+  for (p = abfd->sections; p != NULL; p = p->next)
+    select_data_section(abfd, p, excl_list, adj);
+
   excl_list->sort_and_check ();
 
   bfd_close (abfd);
-- 
2.27.0

Re: [PATCH 0/8] Fix dumper for x86_64

[Jon Turney]

This needs to be aligned with some changes to gdb to consume the dumps 
it produces, so it's probably best to hold off applying this until it's 
more obvious what's going to happen with those.

Random notes:

- objdump identifies the output of dumper on x86_64 as 
'elf64-x86-64-cloudabi' (perhaps due to some over-eager sniffer).

- regions excluded from the dump aren't rounded up to page size, so we 
may end up writing the excess into the dump.

- looking at the loaded modules and inspecting them to determine what 
memory regions don't need to appear in the dump seems odd.  I'm not sure 
we don't just exclude MEMORY_BASIC_INFORMATION.Type == MEM_IMAGE regions 
(assuming they get converted to MEM_PRIVATE regions if written when 
copy-on-write).

Re: [PATCH 8/8] Cygwin: Consider DLL rebasing when computing dumper exclusions

[Corinna Vinschen]

On Jul  1 22:25, Jon Turney wrote:
> I think this would always have been neeeded, but is essential on x86_64,
> as kernel32.dll has an ImageBase of 00000001:80000000 (but is always

Great, but that shouldn't matter much given that system DLLs are
ASLRed all the time.

> +parse_pe (const char *file_name, exclusion * excl_list, LPVOID base_address)
>  {
>    if (file_name == NULL || excl_list == NULL)
>      return 0;
> @@ -104,7 +104,19 @@ parse_pe (const char *file_name, exclusion * excl_list)
>      }
>  
>    bfd_check_format (abfd, bfd_object);
> -  bfd_map_over_sections (abfd, &select_data_section, (PTR) excl_list);
> +
> +  /* Compute the relocation offset for this DLL.  Unfortunately, we have to
> +     guess at ImageBase (one page before vma of first section), since bfd
> +     doesn't let us get at backend-private data */
> +  bfd_vma imagebase = abfd->sections->vma - 0x1000;

VirtualQueryEx?  The AllocationBase is identical to the base address
of the DLL loaded at that address.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: [PATCH 0/8] Fix dumper for x86_64

[Corinna Vinschen]

On Jul  1 22:29, Jon Turney wrote:
> 
> This needs to be aligned with some changes to gdb to consume the dumps it
> produces, so it's probably best to hold off applying this until it's more
> obvious what's going to happen with those.
> 
> Random notes:
> 
> - objdump identifies the output of dumper on x86_64 as
> 'elf64-x86-64-cloudabi' (perhaps due to some over-eager sniffer).
> 
> - regions excluded from the dump aren't rounded up to page size, so we may
> end up writing the excess into the dump.
> 
> - looking at the loaded modules and inspecting them to determine what memory
> regions don't need to appear in the dump seems odd.  I'm not sure we don't
> just exclude MEMORY_BASIC_INFORMATION.Type == MEM_IMAGE regions (assuming
> they get converted to MEM_PRIVATE regions if written when copy-on-write).

Could format_process_maps() in fhandler_process.cc be a role model here?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: [PATCH 0/8] Fix dumper for x86_64

[Corinna Vinschen]

On Jul  1 22:25, Jon Turney wrote:
> Jon Turney (8):
>   Cygwin: Slightly improve error_start documentation
>   Cygwin: Update ELF target used by dumper on x86_64
>   Cygwin: Add a new win32_pstatus data type for modules on x86_64
>   Cygwin: Make dumper scan more than first 4GB of VM on x86_64
>   Cygwin: Fix bfd target for parsing PE files on x86_64 in dumper
>   Cygwin: Fix dumper region order/overlap checking
>   Cygwin: Handle excluded regions more robustly in dumper
>   Cygwin: Consider DLL rebasing when computing dumper exclusions
> 
>  winsup/cygwin/include/cygwin/core_dump.h | 16 ++++++---
>  winsup/doc/cygwinenv.xml                 |  6 +++-
>  winsup/utils/dumper.cc                   | 23 ++++++++++---
>  winsup/utils/dumper.h                    |  2 +-
>  winsup/utils/parse_pe.cc                 | 43 +++++++++++++++++-------
>  5 files changed, 66 insertions(+), 24 deletions(-)
> 
> -- 
> 2.27.0

As soon as you're sure the timing is right, feel free to apply
patches as you see fit.  Dumper is all yours :)


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: [PATCH 8/8] Cygwin: Consider DLL rebasing when computing dumper exclusions

[Corinna Vinschen]

On Jul  2 09:43, Corinna Vinschen wrote:
> On Jul  1 22:25, Jon Turney wrote:
> > I think this would always have been neeeded, but is essential on x86_64,
> > as kernel32.dll has an ImageBase of 00000001:80000000 (but is always
> 
> Great, but that shouldn't matter much given that system DLLs are
> ASLRed all the time.
> 
> > +parse_pe (const char *file_name, exclusion * excl_list, LPVOID base_address)
> >  {
> >    if (file_name == NULL || excl_list == NULL)
> >      return 0;
> > @@ -104,7 +104,19 @@ parse_pe (const char *file_name, exclusion * excl_list)
> >      }
> >  
> >    bfd_check_format (abfd, bfd_object);
> > -  bfd_map_over_sections (abfd, &select_data_section, (PTR) excl_list);
> > +
> > +  /* Compute the relocation offset for this DLL.  Unfortunately, we have to
> > +     guess at ImageBase (one page before vma of first section), since bfd
> > +     doesn't let us get at backend-private data */
> > +  bfd_vma imagebase = abfd->sections->vma - 0x1000;
> 
> VirtualQueryEx?  The AllocationBase is identical to the base address
> of the DLL loaded at that address.

Uhm... right.  Always assuming you get at the Windows process handle
from bfd...


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: [PATCH 8/8] Cygwin: Consider DLL rebasing when computing dumper exclusions

[Jon Turney]

On 02/07/2020 08:48, Corinna Vinschen wrote:
> On Jul  2 09:43, Corinna Vinschen wrote:
>> On Jul  1 22:25, Jon Turney wrote:
>>> I think this would always have been neeeded, but is essential on x86_64,
>>> as kernel32.dll has an ImageBase of 00000001:80000000 (but is always
>>
>> Great, but that shouldn't matter much given that system DLLs are
>> ASLRed all the time.
>>
>>> +parse_pe (const char *file_name, exclusion * excl_list, LPVOID base_address)
>>>   {
>>>     if (file_name == NULL || excl_list == NULL)
>>>       return 0;
>>> @@ -104,7 +104,19 @@ parse_pe (const char *file_name, exclusion * excl_list)
>>>       }
>>>   
>>>     bfd_check_format (abfd, bfd_object);
>>> -  bfd_map_over_sections (abfd, &select_data_section, (PTR) excl_list);
>>> +
>>> +  /* Compute the relocation offset for this DLL.  Unfortunately, we have to
>>> +     guess at ImageBase (one page before vma of first section), since bfd
>>> +     doesn't let us get at backend-private data */
>>> +  bfd_vma imagebase = abfd->sections->vma - 0x1000;
>>
>> VirtualQueryEx?  The AllocationBase is identical to the base address
>> of the DLL loaded at that address.
> 
> Uhm... right.  Always assuming you get at the Windows process handle
> from bfd...

The problem is in the opposite direction.

We have the actual base address the DLL was loaded at in the process 
being dumped, and it's filename, from the LOAD_DLL_DEBUG_EVENT event.

(To my amazement) we then read that DLL using bfd, and examine it for 
sections with the 'CODE' or 'DEBUGGING' flags, the address ranges 
corresponding to which we believe we want to exclude from the dump.

Unfortunately, these addresses are based on the ImageBase in the PE header.

If that's different to the actual base address the PE was loaded at, we 
need to adjust these addresses appropriately.  But libbfd doesn't appear 
to provide a public interface to get at the ImageBase.

Re: [PATCH 8/8] Cygwin: Consider DLL rebasing when computing dumper exclusions

[Corinna Vinschen]

On Jul  3 14:10, Jon Turney wrote:
> On 02/07/2020 08:48, Corinna Vinschen wrote:
> > On Jul  2 09:43, Corinna Vinschen wrote:
> > > On Jul  1 22:25, Jon Turney wrote:
> > > > I think this would always have been neeeded, but is essential on x86_64,
> > > > as kernel32.dll has an ImageBase of 00000001:80000000 (but is always
> > > 
> > > Great, but that shouldn't matter much given that system DLLs are
> > > ASLRed all the time.
> > > 
> > > > +parse_pe (const char *file_name, exclusion * excl_list, LPVOID base_address)
> > > >   {
> > > >     if (file_name == NULL || excl_list == NULL)
> > > >       return 0;
> > > > @@ -104,7 +104,19 @@ parse_pe (const char *file_name, exclusion * excl_list)
> > > >       }
> > > >     bfd_check_format (abfd, bfd_object);
> > > > -  bfd_map_over_sections (abfd, &select_data_section, (PTR) excl_list);
> > > > +
> > > > +  /* Compute the relocation offset for this DLL.  Unfortunately, we have to
> > > > +     guess at ImageBase (one page before vma of first section), since bfd
> > > > +     doesn't let us get at backend-private data */
> > > > +  bfd_vma imagebase = abfd->sections->vma - 0x1000;
> > > 
> > > VirtualQueryEx?  The AllocationBase is identical to the base address
> > > of the DLL loaded at that address.
> > 
> > Uhm... right.  Always assuming you get at the Windows process handle
> > from bfd...
> 
> The problem is in the opposite direction.
> 
> We have the actual base address the DLL was loaded at in the process being
> dumped, and it's filename, from the LOAD_DLL_DEBUG_EVENT event.
> 
> (To my amazement) we then read that DLL using bfd, and examine it for
> sections with the 'CODE' or 'DEBUGGING' flags, the address ranges
> corresponding to which we believe we want to exclude from the dump.
> 
> Unfortunately, these addresses are based on the ImageBase in the PE header.
> 
> If that's different to the actual base address the PE was loaded at, we need
> to adjust these addresses appropriately.  But libbfd doesn't appear to
> provide a public interface to get at the ImageBase.

Ok, but you have the filename, so you can map the file and read it's
header and thus imagebase.  Still not nice, sure... but it would work
without guessing, I guess? :)


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[PATCH 9/8] Add all memory regions details to dumper debug output

[Jon Turney]

---
 winsup/utils/dumper.cc | 101 ++++++++++++++++++++++++++++++++---------
 1 file changed, 80 insertions(+), 21 deletions(-)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index fa3fe1fbc..04b7aa638 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -292,6 +292,25 @@ dumper::add_module (LPVOID base_address)
 
 #define PAGE_BUFFER_SIZE 4096
 
+void protect_dump(DWORD protect, char *buf)
+{
+  const char *pt[10];
+  pt[0] = (protect & PAGE_READONLY) ? "RO " : "";
+  pt[1] = (protect & PAGE_READWRITE) ? "RW " : "";
+  pt[2] = (protect & PAGE_WRITECOPY) ? "WC " : "";
+  pt[3] = (protect & PAGE_EXECUTE) ? "EX " : "";
+  pt[4] = (protect & PAGE_EXECUTE_READ) ? "EXRO " : "";
+  pt[5] = (protect & PAGE_EXECUTE_READWRITE) ? "EXRW " : "";
+  pt[6] = (protect & PAGE_EXECUTE_WRITECOPY) ? "EXWC " : "";
+  pt[7] = (protect & PAGE_GUARD) ? "GRD " : "";
+  pt[8] = (protect & PAGE_NOACCESS) ? "NA " : "";
+  pt[9] = (protect & PAGE_NOCACHE) ? "NC " : "";
+
+  buf[0] = '\0';
+  for (int i = 0; i < 10; i++)
+    strcat (buf, pt[i]);
+}
+
 int
 dumper::collect_memory_sections ()
 {
@@ -316,10 +335,65 @@ dumper::collect_memory_sections ()
 	break;
 
       int skip_region_p = 0;
+      const char *disposition = "dumped";
 
-      if (mbi.Protect & (PAGE_NOACCESS | PAGE_GUARD) ||
-	  mbi.State != MEM_COMMIT)
-	skip_region_p = 1;
+      if (mbi.Protect & PAGE_NOACCESS)
+	{
+	  skip_region_p = 1;
+	  disposition = "skipped due to noaccess";
+	}
+
+      if (mbi.Protect & PAGE_GUARD)
+	{
+	  skip_region_p = 1;
+	  disposition = "skipped due to guardpage";
+	}
+
+      if (mbi.State != MEM_COMMIT)
+	{
+	  skip_region_p = 1;
+	  disposition = "skipped due to uncommited";
+	}
+
+      {
+	char buf[10 * 6];
+	protect_dump(mbi.Protect, buf);
+
+	const char *state = "";
+	const char *type = "";
+
+	if (mbi.State & MEM_COMMIT)
+	  {
+	    state = "COMMIT";
+	  }
+	else if (mbi.State & MEM_FREE)
+	  {
+	    state = "FREE";
+	    type = "FREE";
+	  }
+	else if (mbi.State & MEM_RESERVE)
+	  {
+	    state = "RESERVE";
+	  }
+
+	if (mbi.Type & MEM_IMAGE)
+	  {
+	    type = "IMAGE";
+	  }
+	else if (mbi.Type & MEM_MAPPED)
+	  {
+	    type = "MAPPED";
+	  }
+	else if (mbi.Type & MEM_PRIVATE)
+	  {
+	    type = "PRIVATE";
+	  }
+
+	deb_printf ("region 0x%016lx-0x%016lx (protect = %-8s, state = %-7s, type = %-7s, %s)\n",
+		    current_page_address,
+		    current_page_address + mbi.RegionSize,
+		    buf, state, type, disposition);
+      }
 
       if (!skip_region_p)
 	{
@@ -329,26 +403,11 @@ dumper::collect_memory_sections ()
 	  if (!ReadProcessMemory (hProcess, current_page_address, mem_buf, sizeof (mem_buf), &done))
 	    {
 	      DWORD err = GetLastError ();
-	      const char *pt[10];
-	      pt[0] = (mbi.Protect & PAGE_READONLY) ? "RO " : "";
-	      pt[1] = (mbi.Protect & PAGE_READWRITE) ? "RW " : "";
-	      pt[2] = (mbi.Protect & PAGE_WRITECOPY) ? "WC " : "";
-	      pt[3] = (mbi.Protect & PAGE_EXECUTE) ? "EX " : "";
-	      pt[4] = (mbi.Protect & PAGE_EXECUTE_READ) ? "EXRO " : "";
-	      pt[5] = (mbi.Protect & PAGE_EXECUTE_READWRITE) ? "EXRW " : "";
-	      pt[6] = (mbi.Protect & PAGE_EXECUTE_WRITECOPY) ? "EXWC " : "";
-	      pt[7] = (mbi.Protect & PAGE_GUARD) ? "GRD " : "";
-	      pt[8] = (mbi.Protect & PAGE_NOACCESS) ? "NA " : "";
-	      pt[9] = (mbi.Protect & PAGE_NOCACHE) ? "NC " : "";
-	      char buf[10 * 6];
-	      buf[0] = '\0';
-	      for (int i = 0; i < 10; i++)
-		strcat (buf, pt[i]);
-
-	      deb_printf ("warning: failed to read memory at %p-%p (protect = %s), error %ld.\n",
+
+	      deb_printf ("warning: failed to read memory at %p-%p, error %ld.\n",
 			  current_page_address,
 			  current_page_address + mbi.RegionSize,
-			  buf, err);
+			  err);
 	      skip_region_p = 1;
 	    }
 	}
-- 
2.27.0

[PATCH 10/8] Remove PE reading for section flags from dumper

[Jon Turney]

---
 winsup/utils/Makefile.in |   6 +-
 winsup/utils/dumper.cc   |   2 -
 winsup/utils/dumper.h    |   2 -
 winsup/utils/parse_pe.cc | 124 ---------------------------------------
 4 files changed, 3 insertions(+), 131 deletions(-)
 delete mode 100644 winsup/utils/parse_pe.cc

diff --git a/winsup/utils/Makefile.in b/winsup/utils/Makefile.in
index 5bb62bc6f..f9892fa1d 100644
--- a/winsup/utils/Makefile.in
+++ b/winsup/utils/Makefile.in
@@ -113,9 +113,9 @@ build_dumper := $(shell test -r "$(libbfd)" && echo 1)
 
 ifdef build_dumper
 CYGWIN_BINS += dumper.exe
-dumper.o module_info.o parse_pe.o: CXXFLAGS += -I$(top_srcdir)/include
-dumper.o parse_pe.o: dumper.h
-dumper.exe: module_info.o parse_pe.o
+dumper.o module_info.o: CXXFLAGS += -I$(top_srcdir)/include
+dumper.o: dumper.h
+dumper.exe: module_info.o
 dumper.exe: CYGWIN_LDFLAGS += -lpsapi -lbfd -lintl -liconv -liberty ${ZLIB}
 else
 all: warn_dumper
diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index 04b7aa638..d9d07c055 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -284,8 +284,6 @@ dumper::add_module (LPVOID base_address)
   new_entity->u.module.base_address = base_address;
   new_entity->u.module.name = module_name;
 
-  parse_pe (module_name, excl_list, base_address);
-
   deb_printf ("added module %p %s\n", base_address, module_name);
   return 1;
 }
diff --git a/winsup/utils/dumper.h b/winsup/utils/dumper.h
index 9c4b1f092..78592b61e 100644
--- a/winsup/utils/dumper.h
+++ b/winsup/utils/dumper.h
@@ -133,8 +133,6 @@ extern int deb_printf ( const char* format, ... );
 
 extern char* psapi_get_module_name ( HANDLE hProcess, LPVOID BaseAddress );
 
-extern int parse_pe ( const char* file_name, exclusion* excl_list, LPVOID base_address );
-
 extern BOOL verbose;
 
 #endif
diff --git a/winsup/utils/parse_pe.cc b/winsup/utils/parse_pe.cc
deleted file mode 100644
index 0256ada53..000000000
--- a/winsup/utils/parse_pe.cc
+++ /dev/null
@@ -1,124 +0,0 @@
-/* parse_pe.cc
-
-   Written by Egor Duda <deo@logos-m.ru>
-
-   This file is part of Cygwin.
-
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License (file COPYING.dumper) for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program; if not, write to the Free Software
-   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */
-
-#define PACKAGE
-#include <bfd.h>
-#include <stdio.h>
-#include <stdlib.h>
-
-#include "dumper.h"
-
-#ifndef bfd_get_section_size
-#define bfd_get_section_size(sect) bfd_section_size(sect)
-#endif
-
-int
-exclusion::add (LPBYTE mem_base, SIZE_T mem_size)
-{
-  while (last >= size)
-    size += step;
-  region = (process_mem_region *) realloc (region, size * sizeof (process_mem_region));
-  if (region == NULL)
-    return 0;
-  region[last].base = mem_base;
-  region[last].size = mem_size;
-  last++;
-  return 1;
-};
-
-int
-cmp_regions (const void *r1, const void *r2)
-{
-  if (((process_mem_region *) r1)->base < ((process_mem_region *) r2)->base)
-    return -1;
-  if (((process_mem_region *) r1)->base > ((process_mem_region *) r2)->base)
-    return 1;
-  return 0;
-}
-
-int
-exclusion::sort_and_check ()
-{
-  qsort (region, last, sizeof (process_mem_region), &cmp_regions);
-  for (process_mem_region * p = region; p < region + last - 1; p++)
-    {
-      process_mem_region *q = p + 1;
-      if (p->base + p->size > q->base)
-	{
-	  fprintf (stderr, "region error @ (%p + 0x%0llx) > %p\n", p->base, p->size, q->base);
-	  return 0;
-	}
-    }
-  return 1;
-}
-
-static void
-select_data_section (bfd * abfd, asection * sect, exclusion *excl_list,
-		     SSIZE_T adj)
-{
-  if ((sect->flags & (SEC_CODE | SEC_DEBUGGING)) &&
-      sect->vma && bfd_get_section_size (sect))
-    {
-      bfd_vma vma = sect->vma + adj;
-      excl_list->add ((LPBYTE) vma, (SIZE_T) bfd_get_section_size (sect));
-      deb_printf ("excluding section: %20s %016lx %016lx %08lx\n", sect->name,
-		  sect->vma, vma, bfd_get_section_size (sect));
-    }
-}
-
-int
-parse_pe (const char *file_name, exclusion * excl_list, LPVOID base_address)
-{
-  if (file_name == NULL || excl_list == NULL)
-    return 0;
-
-#ifdef __x86_64__
-  const char *target = "pei-x86-64";
-#else
-  const char *target = "pei-i386";
-#endif
-
-  bfd *abfd = bfd_openr (file_name, target);
-
-  if (abfd == NULL)
-    {
-      bfd_perror ("failed to open file");
-      return 0;
-    }
-
-  bfd_check_format (abfd, bfd_object);
-
-  /* Compute the relocation offset for this DLL.  Unfortunately, we have to
-     guess at ImageBase (one page before vma of first section), since bfd
-     doesn't let us get at backend-private data */
-  bfd_vma imagebase = abfd->sections->vma - 0x1000;
-  SSIZE_T adj = (bfd_vma)base_address - imagebase;
-  deb_printf("imagebase relocated from %016lx to %016lx (%016lx)\n",
-	     imagebase, base_address, adj);
-
-  asection *p;
-  for (p = abfd->sections; p != NULL; p = p->next)
-    select_data_section(abfd, p, excl_list, adj);
-
-  excl_list->sort_and_check ();
-
-  bfd_close (abfd);
-  return 1;
-}
-- 
2.27.0

[PATCH 11/8] Drop excluded regions list from dumper

[Jon Turney]

Drop excluded regions, now it's always empty
---
 winsup/utils/dumper.cc | 51 ++++--------------------------------------
 winsup/utils/dumper.h  | 17 --------------
 2 files changed, 4 insertions(+), 64 deletions(-)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index d9d07c055..2a0c66002 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -83,7 +83,6 @@ dumper::dumper (DWORD pid, DWORD tid, const char *file_name)
   this->pid = pid;
   this->tid = tid;
   core_bfd = NULL;
-  excl_list = new exclusion (20);
 
   list = last = NULL;
 
@@ -125,19 +124,16 @@ dumper::close ()
 {
   if (core_bfd)
     bfd_close (core_bfd);
-  if (excl_list)
-    delete excl_list;
   if (hProcess)
     CloseHandle (hProcess);
   core_bfd = NULL;
   hProcess = NULL;
-  excl_list = NULL;
 }
 
 int
 dumper::sane ()
 {
-  if (hProcess == NULL || core_bfd == NULL || excl_list == NULL)
+  if (hProcess == NULL || core_bfd == NULL)
     return 0;
   return 1;
 }
@@ -226,45 +222,6 @@ dumper::add_mem_region (LPBYTE base, SIZE_T size)
   return 1;
 }
 
-/* split_add_mem_region scans list of regions to be excluded from dumping process
-   (excl_list) and removes all "excluded" parts from given region. */
-int
-dumper::split_add_mem_region (LPBYTE base, SIZE_T size)
-{
-  if (!sane ())
-    return 0;
-
-  if (base == NULL || size == 0)
-    return 1;			// just ignore empty regions
-
-  LPBYTE last_base = base;
-
-  for (process_mem_region * p = excl_list->region;
-       p < excl_list->region + excl_list->last;
-       p++)
-    {
-      if (p->base >= base + size || p->base + p->size <= base)
-	continue;
-
-      if (p->base <= base)
-	{
-	  last_base = p->base + p->size;
-	  continue;
-	}
-
-      if (p->base < last_base)
-	continue;
-
-      add_mem_region (last_base, p->base - last_base);
-      last_base = p->base + p->size;
-    }
-
-  if (last_base < base + size)
-    add_mem_region (last_base, base + size - last_base);
-
-  return 1;
-}
-
 int
 dumper::add_module (LPVOID base_address)
 {
@@ -416,14 +373,14 @@ dumper::collect_memory_sections ()
 	    last_size += mbi.RegionSize;
 	  else
 	    {
-	      split_add_mem_region (last_base, last_size);
+	      add_mem_region (last_base, last_size);
 	      last_base = (LPBYTE) mbi.BaseAddress;
 	      last_size = mbi.RegionSize;
 	    }
 	}
       else
 	{
-	  split_add_mem_region (last_base, last_size);
+	  add_mem_region (last_base, last_size);
 	  last_base = NULL;
 	  last_size = 0;
 	}
@@ -432,7 +389,7 @@ dumper::collect_memory_sections ()
     }
 
   /* dump last sections, if any */
-  split_add_mem_region (last_base, last_size);
+  add_mem_region (last_base, last_size);
   return 1;
 };
 
diff --git a/winsup/utils/dumper.h b/winsup/utils/dumper.h
index 78592b61e..6e624a983 100644
--- a/winsup/utils/dumper.h
+++ b/winsup/utils/dumper.h
@@ -62,22 +62,6 @@ typedef struct _process_entity
   struct _process_entity* next;
 } process_entity;
 
-class exclusion
-{
-public:
-  size_t last;
-  size_t size;
-  size_t step;
-  process_mem_region* region;
-
-  exclusion ( size_t step ) { last = size = 0;
-			      this->step = step;
-			      region = NULL; }
-  ~exclusion () { free ( region ); }
-  int add ( LPBYTE mem_base, SIZE_T mem_size );
-  int sort_and_check ();
-};
-
 #define PAGE_BUFFER_SIZE 4096
 
 class dumper
@@ -87,7 +71,6 @@ class dumper
   HANDLE hProcess;
   process_entity* list;
   process_entity* last;
-  exclusion* excl_list;
 
   char* file_name;
   bfd* core_bfd;
-- 
2.27.0

[PATCH 12/8] Don't dump non-writable image regions

[Jon Turney]

After this, we will end up dumping memory regions where:

- state is MEM_COMMIT (i.e. is not MEM_RESERVE or MEM_FREE), and
-- type is MEM_PRIVATE and protection allows reads (i.e. not a guardpage), or
-- type is MEM_IMAGE and protection allows writes

Making this decision based on the current protection isn't 100% correct,
because it may have been changed using VirtualProtect().  But we don't
know how to determine if a region is shareable.

(As a practical matter, anything which gets us the stack (MEM_PRIVATE)
and .data/.bss (RW MEM_IMAGE) is going to be enough for 99% of cases)
---
 winsup/utils/dumper.cc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index 2a0c66002..b96ee54cc 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -292,6 +292,12 @@ dumper::collect_memory_sections ()
       int skip_region_p = 0;
       const char *disposition = "dumped";
 
+      if ((mbi.Type & MEM_IMAGE) && !(mbi.Protect & (PAGE_EXECUTE_READWRITE | PAGE_READWRITE)))
+	{
+	  skip_region_p = 1;
+	  disposition = "skipped due to non-writeable MEM_IMAGE";
+	}
+
       if (mbi.Protect & PAGE_NOACCESS)
 	{
 	  skip_region_p = 1;
-- 
2.27.0

Re: [PATCH 0/8] Fix dumper for x86_64

[Jon Turney]

On 02/07/2020 08:44, Corinna Vinschen wrote:
> On Jul  1 22:29, Jon Turney wrote:
>>
>> This needs to be aligned with some changes to gdb to consume the dumps it
>> produces, so it's probably best to hold off applying this until it's more
>> obvious what's going to happen with those.
>>
>> Random notes:
>>
>> - objdump identifies the output of dumper on x86_64 as
>> 'elf64-x86-64-cloudabi' (perhaps due to some over-eager sniffer).
>>
>> - regions excluded from the dump aren't rounded up to page size, so we may
>> end up writing the excess into the dump.
>>
>> - looking at the loaded modules and inspecting them to determine what memory
>> regions don't need to appear in the dump seems odd.  I'm not sure we don't
>> just exclude MEMORY_BASIC_INFORMATION.Type == MEM_IMAGE regions (assuming
>> they get converted to MEM_PRIVATE regions if written when copy-on-write).

Unfortunately, that doesn't happen, and the region appears to stay 
MEM_IMAGE, even if it's been modified.

I'm inclined to just dump MEM_IMAGE regions if they are writable 
(although using the current protection isn't 100% correct, because it 
may have been changed using VirtualProtect())

I suspect there's probably some undocumented MemoryInformationClass for 
NtQueryVirtualMemory() that would let us determine if a region is 
sharable or not, but ...

> Could format_process_maps() in fhandler_process.cc be a role model here?

Thanks for pointing that out.

Re: [PATCH 0/8] Fix dumper for x86_64

[Corinna Vinschen]

On Jul  5 17:49, Jon Turney wrote:
> On 02/07/2020 08:44, Corinna Vinschen wrote:
> > On Jul  1 22:29, Jon Turney wrote:
> > > 
> > > This needs to be aligned with some changes to gdb to consume the dumps it
> > > produces, so it's probably best to hold off applying this until it's more
> > > obvious what's going to happen with those.
> > > 
> > > Random notes:
> > > 
> > > - objdump identifies the output of dumper on x86_64 as
> > > 'elf64-x86-64-cloudabi' (perhaps due to some over-eager sniffer).
> > > 
> > > - regions excluded from the dump aren't rounded up to page size, so we may
> > > end up writing the excess into the dump.
> > > 
> > > - looking at the loaded modules and inspecting them to determine what memory
> > > regions don't need to appear in the dump seems odd.  I'm not sure we don't
> > > just exclude MEMORY_BASIC_INFORMATION.Type == MEM_IMAGE regions (assuming
> > > they get converted to MEM_PRIVATE regions if written when copy-on-write).
> 
> Unfortunately, that doesn't happen, and the region appears to stay
> MEM_IMAGE, even if it's been modified.
> 
> I'm inclined to just dump MEM_IMAGE regions if they are writable (although
> using the current protection isn't 100% correct, because it may have been
> changed using VirtualProtect())
> 
> I suspect there's probably some undocumented MemoryInformationClass for
> NtQueryVirtualMemory() that would let us determine if a region is sharable
> or not, but ...

Surprisingly, there's nothing undocumented in NtQueryVirtualMemory and
the API is fully exposed by VirtualQuery(Ex).

What about the two protection fields in MEMORY_BASIC_INFORMATION?  If
something changed, Protect != AllocationProtect.  Is that insufficient
to handle your case?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: [PATCH 0/8] Fix dumper for x86_64

[Jon Turney]

On 06/07/2020 09:12, Corinna Vinschen wrote:
> On Jul  5 17:49, Jon Turney wrote:
>> On 02/07/2020 08:44, Corinna Vinschen wrote:
>>> On Jul  1 22:29, Jon Turney wrote:
>>>>
>>>> This needs to be aligned with some changes to gdb to consume the dumps it
>>>> produces, so it's probably best to hold off applying this until it's more
>>>> obvious what's going to happen with those.
>>>>
>>>> Random notes:
>>>>
>>>> - objdump identifies the output of dumper on x86_64 as
>>>> 'elf64-x86-64-cloudabi' (perhaps due to some over-eager sniffer).
>>>>
>>>> - regions excluded from the dump aren't rounded up to page size, so we may
>>>> end up writing the excess into the dump.
>>>>
>>>> - looking at the loaded modules and inspecting them to determine what memory
>>>> regions don't need to appear in the dump seems odd.  I'm not sure we don't
>>>> just exclude MEMORY_BASIC_INFORMATION.Type == MEM_IMAGE regions (assuming
>>>> they get converted to MEM_PRIVATE regions if written when copy-on-write).
>>
>> Unfortunately, that doesn't happen, and the region appears to stay
>> MEM_IMAGE, even if it's been modified.
>>
>> I'm inclined to just dump MEM_IMAGE regions if they are writable (although
>> using the current protection isn't 100% correct, because it may have been
>> changed using VirtualProtect())
>>
>> I suspect there's probably some undocumented MemoryInformationClass for
>> NtQueryVirtualMemory() that would let us determine if a region is sharable
>> or not, but ...
> 
> Surprisingly, there's nothing undocumented in NtQueryVirtualMemory and
> the API is fully exposed by VirtualQuery(Ex).

I came across [1], which seems to use some MemoryInformationClass values 
I can't find any MSDN documentation on, but perhaps I'm getting lost.

[1] 
https://github.com/processhacker/processhacker/blob/master/phnt/include/ntmmapi.h#L87

> What about the two protection fields in MEMORY_BASIC_INFORMATION?  If
> something changed, Protect != AllocationProtect.  Is that insufficient
> to handle your case?

Unfortunately that doesn't seem to provide any additional information. 
The Windows loader seems to allocate all regions with EXWC protection, 
then change it to match the section. (Not that there are any guarantees 
about it's behaviour)

I wasn't able to observe a region corresponding to an unmodified .data 
section with WC protection, which is somewhat confusing.

Re: [PATCH 0/8] Fix dumper for x86_64

[Corinna Vinschen]

On Jul  6 14:34, Jon Turney wrote:
> On 06/07/2020 09:12, Corinna Vinschen wrote:
> > On Jul  5 17:49, Jon Turney wrote:
> > > On 02/07/2020 08:44, Corinna Vinschen wrote:
> > > > On Jul  1 22:29, Jon Turney wrote:
> > > > > 
> > > > > This needs to be aligned with some changes to gdb to consume the dumps it
> > > > > produces, so it's probably best to hold off applying this until it's more
> > > > > obvious what's going to happen with those.
> > > > > 
> > > > > Random notes:
> > > > > 
> > > > > - objdump identifies the output of dumper on x86_64 as
> > > > > 'elf64-x86-64-cloudabi' (perhaps due to some over-eager sniffer).
> > > > > 
> > > > > - regions excluded from the dump aren't rounded up to page size, so we may
> > > > > end up writing the excess into the dump.
> > > > > 
> > > > > - looking at the loaded modules and inspecting them to determine what memory
> > > > > regions don't need to appear in the dump seems odd.  I'm not sure we don't
> > > > > just exclude MEMORY_BASIC_INFORMATION.Type == MEM_IMAGE regions (assuming
> > > > > they get converted to MEM_PRIVATE regions if written when copy-on-write).
> > > 
> > > Unfortunately, that doesn't happen, and the region appears to stay
> > > MEM_IMAGE, even if it's been modified.
> > > 
> > > I'm inclined to just dump MEM_IMAGE regions if they are writable (although
> > > using the current protection isn't 100% correct, because it may have been
> > > changed using VirtualProtect())
> > > 
> > > I suspect there's probably some undocumented MemoryInformationClass for
> > > NtQueryVirtualMemory() that would let us determine if a region is sharable
> > > or not, but ...
> > 
> > Surprisingly, there's nothing undocumented in NtQueryVirtualMemory and
> > the API is fully exposed by VirtualQuery(Ex).
> 
> I came across [1], which seems to use some MemoryInformationClass values I
> can't find any MSDN documentation on, but perhaps I'm getting lost.
> 
> [1] https://github.com/processhacker/processhacker/blob/master/phnt/include/ntmmapi.h#L87

Uh, sorry.  I confused NtQueryVirtualMemory with just the
MemoryBasicInformation class.  Looking into the above, the
MEMORY_REGION_INFORMATION struct looks pretty interesting
but I doubt it helps...


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: [PATCH 0/8] Fix dumper for x86_64

[Jon Turney]

On 06/07/2020 14:34, Jon Turney wrote:
> On 06/07/2020 09:12, Corinna Vinschen wrote: 
> 
>> What about the two protection fields in MEMORY_BASIC_INFORMATION?  If
>> something changed, Protect != AllocationProtect.  Is that insufficient
>> to handle your case?
> 
> Unfortunately that doesn't seem to provide any additional information. 
> The Windows loader seems to allocate all regions with EXWC protection, 
> then change it to match the section. (Not that there are any guarantees 
> about it's behaviour)
> 
> I wasn't able to observe a region corresponding to an unmodified .data 
> section with WC protection, which is somewhat confusing.

I guess that might be due to something in crt0 modifying .data, since 
testing with something like:

      1  #include <windows.h>
      2
      3  int __attribute__ ((section (".special"))) mutable = 2;
      4
      5  int main()
      6  {
      7    // modify rw data
      8    // mutable = 0;
      9
     10    // deref null pointer
     11    *(int *)0 = 1;
     12  }

The memory region corresponding to the '.special' section has WC 
protection, which changes to RW if it gets modified (as expected).