On Tuesday 03 December 2024 16:35:38 Martin Storsjö wrote:
> On Fri, 29 Nov 2024, Pali Rohár wrote:
>
> > PE format allows to have variable length of Data Directories in PE Optional
> > Header. The exact number of Data Directories is stored in
> > NumberOfRvaAndSizes
> > field. Size of the optional header depends on the number of Data
> > Directories.
> >
> > Constants IMAGE_SIZEOF_NT_OPTIONAL32_HEADER and
> > IMAGE_SIZEOF_NT_OPTIONAL64_HEADER
> > are for PE images with all 16 Data Directories. And so cannot be used for
> > checking validity of SizeOfOptionalHeader.
> >
> > Older PE linkers were generating PE binaries with less number of Data
> > Directories than 16 if trailing entries were empty. And gendef cannot
> > recognize such PE binaries.
> >
> > Relax check for PE SizeOfOptionalHeader field. Allow any number of Data
> > Directories in PE Optional Header, including zero.
> > ---
> > mingw-w64-tools/gendef/src/gendef.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/mingw-w64-tools/gendef/src/gendef.c
> > b/mingw-w64-tools/gendef/src/gendef.c
> > index 7440d60f35df..ae9603fe1a3a 100644
> > --- a/mingw-w64-tools/gendef/src/gendef.c
> > +++ b/mingw-w64-tools/gendef/src/gendef.c
> > @@ -313,19 +313,19 @@ load_pep (void)
> > gPEDta = NULL;
> > return 0;
> > }
> > - if (gPEDta->FileHeader.SizeOfOptionalHeader ==
> > IMAGE_SIZEOF_NT_OPTIONAL32_HEADER
> > + if (gPEDta->FileHeader.SizeOfOptionalHeader >=
> > IMAGE_SIZEOF_NT_OPTIONAL32_HEADER-sizeof(gPEDta->OptionalHeader.DataDirectory)
>
> This looks ok, but the expression
>
> IMAGE_SIZEOF_NT_OPTIONAL32_HEADER -
> sizeof(gPEDta->OptionalHeader.DataDirectory)
>
> feels somewhat clumsy; is there any more concise way of expressing this,
> like some sort of offsetof() or so?
Via offsetof() it can be expressed as:
offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory)
It is just stupid definition of the structure which looks like:
#define IMAGE_NUMBEROF_DIRECTORY_ENTRIES 16
typedef struct _IMAGE_OPTIONAL_HEADER {
WORD Magic;
...
DWORD NumberOfRvaAndSizes;
IMAGE_DATA_DIRECTORY DataDirectory[IMAGE_NUMBEROF_DIRECTORY_ENTRIES];
} IMAGE_OPTIONAL_HEADER32,*PIMAGE_OPTIONAL_HEADER32;
#define IMAGE_SIZEOF_NT_OPTIONAL32_HEADER 224
Where sizeof(IMAGE_OPTIONAL_HEADER32) == 224
Member NumberOfRvaAndSizes specifies number of DataDirectory[] items
and based on this is calculated SizeOfOptionalHeader value in upper
structure. Also it is theoretically possible to have more than 16 items,
Windows PE loader just do not use additional one (as it does not
understand them) and still accepts to load & execute such executable.
The correct structure definition (in C99) should have been with:
IMAGE_DATA_DIRECTORY DataDirectory[];
And then sizeof() would have return the minimal size of the structure as
it is the way how variable-length structures are being used.
But this cannot be changed because of API / ABI compatibility.
So the only way is to manually decrease sizeof() value as I have done in
the change.
If you think that offsetof() variant is better, then I can change it no
problem. I just think that this check does not have any good/readable
solution as the structure is incorrectly defined and it structure cannot
be changed/fixed at all.
> The functionality of the patch itself seems fine though.
>
> (I know there are a few other older patchsets I haven't commented on yet,
> but I wanted to reply to this one sooner, as it seemed quicker to review.)
>
> // Martin
No problem, just look at them later when you have a time.
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
