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