This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] [BFD] Add support for reading msdos (MZ) executables.
- From: Nick Clifton <nickc at redhat dot com>
- To: Zebediah Figura <z dot figura12 at gmail dot com>, binutils at sourceware dot org
- Date: Tue, 2 Jan 2018 12:46:48 +0000
- Subject: Re: [PATCH] [BFD] Add support for reading msdos (MZ) executables.
- Authentication-results: sourceware.org; auth=none
- References: <1514828387-6061-1-git-send-email-z.figura12@gmail.com>
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