This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [BFD] Add support for reading msdos (MZ) executables.


On 01/02/2018 06:46 AM, Nick Clifton wrote:
> Hi Zebediah,
> 
>> This is my first time contributing to this project, so patience would be
>> appreciated.
> 
> No worries, and thanks very much for contributing.
> 
> First off - do you have an FSF copyright assignment for the binutils project ?
> If not, you will need to get one before we can accept this patch.
> 

I don't. I will get on this.

> 
>> +struct external_DOS_hdr
>> +{
>> +  /* DOS header fields - always at offset zero in the EXE file.  */
>> +  char e_magic[2];		/* Magic number, 0x5a4d.  */
>> +  char e_cblp[2];		/* Bytes on last page of file, 0x90.  */
>> +  char e_cp[2];			/* Pages in file, 0x3.  */
>> +  char e_crlc[2];		/* Relocations, 0x0.  */
>> +  char e_cparhdr[2];		/* Size of header in paragraphs, 0x4.  */
>> +  char e_minalloc[2];		/* Minimum extra paragraphs needed, 0x0.  */
>> +  char e_maxalloc[2];		/* Maximum extra paragraphs needed, 0xFFFF.  */
>> +  char e_ss[2];			/* Initial (relative) SS value, 0x0.  */
>> +  char e_sp[2];			/* Initial SP value, 0xb8.  */
>> +  char e_csum[2];		/* Checksum, 0x0.  */
>> +  char e_ip[2];			/* Initial IP value, 0x0.  */
>> +  char e_cs[2];			/* Initial (relative) CS value, 0x0.  */
>> +  char e_lfarlc[2];		/* File address of relocation table, 0x40.  */
>> +  char e_ovno[2];		/* Overlay number, 0x0.  */
>> +  char e_res[4][2];		/* Reserved words, all 0x0.  */
>> +  char e_oemid[2];		/* OEM identifier (for e_oeminfo), 0x0.  */
>> +  char e_oeminfo[2];		/* OEM information; e_oemid specific, 0x0.  */
>> +  char e_res2[10][2];		/* Reserved words, all 0x0.  */
>> +  char e_lfanew[4];		/* File address of new exe header, usually 0x80.  */
>> +};
> 
> This is awfully similar to the external_PEI_DOS_hdr structure defined in
> include/coff/pe.h.  Is it possible to use that definition instead (and so
> prevent code duplication) ?  Alternatively, maybe your definition should be
> moved into coff/pe.h or a similar header file, so that it is available to 
> other parts of the binutils...
> 
> 

That's a good point; I'll move this into a common header. Should this be
done in a separate patch?

> 
>> +  /* Check that this isn't actually a PE, NE, or LE file.  */
>> +  if (bfd_seek (abfd, (file_ptr) H_GET_32 (abfd, hdr.e_lfanew), SEEK_SET) != 0
>> +      || bfd_bread (buffer, (bfd_size_type) 2, abfd) != 2)
>> +    {
>> +      if (bfd_get_error () == bfd_error_system_call)
>> +	return NULL;
>> +    }
> 
> I think that you always want to return NULL here.  My guess is that you
> meant something like:
> 
>          if (bfd_get_error () != bfd_error_system_call)
>            bfd_set_errror (bfd_error_wrong_format);
>          return NULL;
> 

I don't think so? For example, if the offset refers to an absurdly large
value (interpreted as such) then bfd_seek() will return
bfd_error_file_truncated, but we still want to treat it as an MZ file.
The e_lfanew field isn't actually part of the MZ header as such, so in a
real file it could hold any sort of data (for example, relocations,
which we don't currently handle).

>> +  else
>> +    {
>> +      if (H_GET_16 (abfd, buffer) == 0x4550	/* PE */
>> +	  || H_GET_16 (abfd, buffer) == 0x454e	/* NE */
>> +	  || H_GET_16 (abfd, buffer) == 0x454c)	/* LE */
> 
> Magic constants - ugg!  Please could you use #define'd values with readable
> names, and then put the actual values into a header file.
> 

Will do.

> 
>> +  size = (H_GET_16 (abfd, hdr.e_cp) - 1) * EXE_PAGE_SIZE - section->filepos;
>> +  size += H_GET_16 (abfd, hdr.e_cblp);
>> +  bfd_set_section_size (abfd, section, size);
> 
> Paranoia - I highly recommend checking the size value before installing it
> into the bfd.  Someone somewhere is bound to try to break the code by providing
> a corrupt DOS header...
> 

Will do, thanks for the catch.

> 
> Cheers
>   Nick
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]