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.


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.


> +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...



> +  /* 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;

> +  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.


> +  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...


Cheers
  Nick


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