GNU bug report logs - #20264
[PATCH] fix: w32_executable_type() causes a segmentation fault

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20264 in the body.
You can then email your comments to 20264 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] fix: w32_executable_type() causes a segmentation fault
Date: Mon, 06 Apr 2015 12:23:23 +0900
 (東京 (標準時))

Hi folks,

On Windows platform, w32_executable_type() in src/w32proc.c scans
'dllname' in an EXE file. But there are some strange EXE files that
'dllname' points to an illegal address, for example, Microsoft's Excel
(excel.exe) and PowerPoint (POWEPNT.EXE). w32_executable_type() causes
a segmentation fault for those files.

objdump in binutils seems to know those illegal pointers and discard
them (pe_print_idata() in bfd/peXXigen.c).

In the following patch, 'dllname' is checked whether it points to the
valid section's address space and discarded when it's invalid.

Regards,
Koichi Arakawa

diff --git a/src/ChangeLog b/src/ChangeLog
index 1c3f933..a49fdf4 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2015-04-06  Koichi Arakawa  <arakawa <at> pp.iij4u.or.jp>
+
+	* w32proc.c (w32_executable_type): Check whether 'dllname' points
+	to the section's address space.
+
 2015-04-04  Jan Djärv  <jan.h.d <at> swipnet.se>
 
 	* xselect.c (x_reply_selection_request)
diff --git a/src/w32proc.c b/src/w32proc.c
index 7d982f8..d3d9405 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -1618,16 +1618,23 @@ w32_executable_type (char * filename,
                 data_dir[IMAGE_DIRECTORY_ENTRY_IMPORT];
               IMAGE_IMPORT_DESCRIPTOR * imports;
               IMAGE_SECTION_HEADER * section;
+              char * base;
+              DWORD_PTR real_size;
 
               section = rva_to_section (import_dir.VirtualAddress, nt_header);
               imports = RVA_TO_PTR (import_dir.VirtualAddress, section,
                                     executable);
+              base = RVA_TO_PTR (section->VirtualAddress, section, executable);
+              real_size = max (section->SizeOfRawData, section->Misc.VirtualSize);
 
               for ( ; imports->Name; imports++)
                 {
                   char * dllname = RVA_TO_PTR (imports->Name, section,
                                                executable);
 
+                  if (imports->Name < base || dllname >= base + real_size)
+                    break;
+
                   /* The exact name of the cygwin dll has changed with
                      various releases, but hopefully this will be reasonably
                      future proof.  */

Message #8 received at 20264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
Cc: 20264 <at> debbugs.gnu.org
Subject: Re: bug#20264: [PATCH] fix: w32_executable_type() causes a
 segmentation	fault
Date: Mon, 06 Apr 2015 11:02:47 +0300

> Date: Mon, 06 Apr 2015 12:23:23 +0900 (東京
> 	(標準時))
> From: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
> 
> On Windows platform, w32_executable_type() in src/w32proc.c scans
> 'dllname' in an EXE file. But there are some strange EXE files that
> 'dllname' points to an illegal address, for example, Microsoft's Excel
> (excel.exe) and PowerPoint (POWEPNT.EXE). w32_executable_type() causes
> a segmentation fault for those files.
> 
> objdump in binutils seems to know those illegal pointers and discard
> them (pe_print_idata() in bfd/peXXigen.c).
> 
> In the following patch, 'dllname' is checked whether it points to the
> valid section's address space and discarded when it's invalid.

Thanks.

>                for ( ; imports->Name; imports++)
>                  {
>                    char * dllname = RVA_TO_PTR (imports->Name, section,
>                                                 executable);
>  
> +                  if (imports->Name < base || dllname >= base + real_size)
> +                    break;
> +

Shouldn't that "break" be "continue" instead?  IOW, shouldn't we try
all the other entries in the DLL import list?

Message #11 received at 20264 <at> debbugs.gnu.org (full text, mbox):

From: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
To: eliz <at> gnu.org
Cc: 20264 <at> debbugs.gnu.org
Subject: Re: bug#20264: [PATCH] fix: w32_executable_type() causes a
 segmentation fault
Date: Mon, 06 Apr 2015 18:48:11 +0900
 (東京 (標準時))

Eli Zaretskii <eliz <at> gnu.org> wrote:
>>                for ( ; imports->Name; imports++)
>>                  {
>>                    char * dllname = RVA_TO_PTR (imports->Name, section,
>>                                                 executable);
>>  
>> +                  if (imports->Name < base || dllname >= base + real_size)
>> +                    break;
>> +
> 
> Shouldn't that "break" be "continue" instead?  IOW, shouldn't we try
> all the other entries in the DLL import list?

I apologize insufficient research. The *illegal* dllname actually
points to another section. So the previous patch is wrong and it
should be as follows.

diff --git a/src/w32proc.c b/src/w32proc.c
index 7d982f8..5ae55ff 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -1625,6 +1625,7 @@ w32_executable_type (char * filename,
 
               for ( ; imports->Name; imports++)
                 {
+                  section = rva_to_section (imports->Name, nt_header);
                   char * dllname = RVA_TO_PTR (imports->Name, section,
                                                executable);
 
-- 
Koichi Arakawa

Message #16 received at 20264-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
Cc: 20264-done <at> debbugs.gnu.org
Subject: Re: bug#20264: [PATCH] fix: w32_executable_type() causes a
 segmentation fault
Date: Mon, 06 Apr 2015 13:30:11 +0300

> Date: Mon, 06 Apr 2015 18:48:11 +0900
>  (東京 (標準時))
> Cc: 20264 <at> debbugs.gnu.org
> From: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
> 
> I apologize insufficient research. The *illegal* dllname actually
> points to another section. So the previous patch is wrong and it
> should be as follows.

Thanks, I pushed it.

Message #19 received at 20264-done <at> debbugs.gnu.org (full text, mbox):

From: Koichi Arakawa <arakawa <at> pp.iij4u.or.jp>
To: eliz <at> gnu.org
Cc: 20264-done <at> debbugs.gnu.org
Subject: Re: bug#20264: [PATCH] fix: w32_executable_type() causes a
 segmentation fault
Date: Tue, 07 Apr 2015 02:04:58 +0900
 (東京 (標準時))

Eli Zaretskii <eliz <at> gnu.org> wrote:
>> I apologize insufficient research. The *illegal* dllname actually
>> points to another section. So the previous patch is wrong and it
>> should be as follows.
> 
> Thanks, I pushed it.

Thank you. I think it works fine.
-- 
Koichi Arakawa

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.