imagehlp: Implement (parts of) BindImageEx

[Bernhard Reiter]

Now that the test has been committed, I'd be grateful for any reviews of
the implementation!

Bernhard

[Erich E. Hoover]

On Fri, Aug 8, 2014 at 8:30 AM, Bernhard Reiter <ockham(a)raz.or.at> wrote:
> Now that the test has been committed, I'd be grateful for any reviews of
> the implementation!

Shouldn't you be calling UnMapAndLoad in the early exit cases?

Best,
Erich

[Bernhard Reiter]

Am 2014-08-08 um 17:26 schrieb Erich E. Hoover:
> Shouldn't you be calling UnMapAndLoad in the early exit cases?

Yeah, you're right. Thx, fixed.

Bernhard

[Bernhard Reiter]

Here's another iteration with indentation fixed and one char* changed to
a const char*. Good enough for wine-patches@ ?

Bernhard

Am 2014-08-08 um 18:34 schrieb Bernhard Reiter:
> Am 2014-08-08 um 17:26 schrieb Erich E. Hoover:
> > Shouldn't you be calling UnMapAndLoad in the early exit cases?
>
> Yeah, you're right. Thx, fixed.
>
> Bernhard

[Thomas Faber]

On 2014-08-12 17:34, Bernhard Reiter wrote:
> +void * WINAPI ImageDirectoryEntryToDataEx( void *base, BOOL image, unsigned short dir,
> +                                           unsigned long *size, PIMAGE_SECTION_HEADER *section );

BOOLEAN is not BOOL (neither on Windows nor on Wine).
LONG is not long (on Wine).
(Including dbghelp.h would complain about this. Not sure why you're not
 doing that, but I haven't looked in detail)
> +      if(thunk->u1.Ordinal & IMAGE_ORDINAL_FLAG)
> +      {
> +        /* Ordinal, i.e. unnamed function. Not sure if/how to pass this on to StatusRoutine... */
> +        /* DWORD ordinal = (DWORD) (thunk->u1.Ordinal & (~IMAGE_ORDINAL_FLAG)); */
> +      }

Most functions I know simply do ((PCSTR)(DWORD_PTR)ordinal) to represent
ordinals (e.g. GetProcAddress). That's just a guess though, I know
nothing about this function.

Best,
Thomas

[Bernhard Reiter]

Am 2014-08-12 um 18:13 schrieb Thomas Faber:
> On 2014-08-12 17:34, Bernhard Reiter wrote:
> > +void * WINAPI ImageDirectoryEntryToDataEx( void *base, BOOL image, unsigned short dir,
> > +                                           unsigned long *size, PIMAGE_SECTION_HEADER *section );
>
> BOOLEAN is not BOOL (neither on Windows nor on Wine).
> LONG is not long (on Wine).

But I've substituted a PULONG here for an unsigned long * -- is that
wrong? Trying an unsigned int* instead gives a compiler warning...
> (Including dbghelp.h would complain about this. Not sure why you're not
>  doing that, but I haven't looked in detail)

Including dbghelp.h along with imagehlp.h unfortunately gives a host of
definition collisions. From MSDN I know that some of dbghelp's functions
are mirrored in imagehlp, but apparently something's wrong here in Wine...
> > +      if(thunk->u1.Ordinal & IMAGE_ORDINAL_FLAG)
> > +      {
> > +        /* Ordinal, i.e. unnamed function. Not sure if/how to pass this on to StatusRoutine... */
> > +        /* DWORD ordinal = (DWORD) (thunk->u1.Ordinal & (~IMAGE_ORDINAL_FLAG)); */
> > +      }
>
> Most functions I know simply do ((PCSTR)(DWORD_PTR)ordinal) to represent
> ordinals (e.g. GetProcAddress). That's just a guess though, I know
> nothing about this function.

I don't know that much either about this stuff -- documentation is
rather scarce. So the only way to find out is more tests, isn't it?
> Best,
> Thomas

Thx for your comments!
Bernhard

[Thomas Faber]

On 2014-08-12 19:34, Bernhard Reiter wrote:
> But I've substituted a PULONG here for an unsigned long * -- is that
> wrong? Trying an unsigned int* instead gives a compiler warning...

The long type on x64 Windows is 32 bit, on x64 Linux (and most other
platforms) it is 64 bit.
To deal with that difference, Wine defines LONG to be int (and ULONG
to be unsigned int etc), which is 32 bit on all the relevant platforms.

So yes, substituting PULONG for unsigned long * is wrong, it needs to
be ULONG *, DWORD *, or similar. The compiler warning that you get from
the correct prototype would be because you're also using a (lowercase)
long when calling the function -- this also needs to be an uppercase
LONG.
> Including dbghelp.h along with imagehlp.h unfortunately gives a host of
> definition collisions. From MSDN I know that some of dbghelp's functions
> are mirrored in imagehlp, but apparently something's wrong here in Wine...

If this works in PSDK, then a fix for Wine's headers is probably
preferable -- but I have no idea how intrusive that would be, so a
workaround like this could be appropriate for the time being.
> > Most functions I know simply do ((PCSTR)(DWORD_PTR)ordinal) to represent
> > ordinals (e.g. GetProcAddress). That's just a guess though, I know
> > nothing about this function.
>
> I don't know that much either about this stuff -- documentation is
> rather scarce. So the only way to find out is more tests, isn't it?

That sounds right ;)  (although a step-by-step approach should be
okay, so I don't think you have to implement this right away)

[Bernhard Reiter]

Am 2014-08-16 um 14:02 schrieb Thomas Faber:
> On 2014-08-12 19:34, Bernhard Reiter wrote:
> > But I've substituted a PULONG here for an unsigned long * -- is that
> > wrong? Trying an unsigned int* instead gives a compiler warning...
>
> The long type on x64 Windows is 32 bit, on x64 Linux (and most other
> platforms) it is 64 bit.
> To deal with that difference, Wine defines LONG to be int (and ULONG
> to be unsigned int etc), which is 32 bit on all the relevant platforms.
>
> So yes, substituting PULONG for unsigned long * is wrong, it needs to
> be ULONG *, DWORD *, or similar. The compiler warning that you get from
> the correct prototype would be because you're also using a (lowercase)
> long when calling the function -- this also needs to be an uppercase
> LONG.

Okay, thanks for making that clear. I tried to look up this stuff myself
but apparently didn't succeed...
> > Including dbghelp.h along with imagehlp.h unfortunately gives a host of
> > definition collisions. From MSDN I know that some of dbghelp's functions
> > are mirrored in imagehlp, but apparently something's wrong here in Wine...
>
> If this works in PSDK, then a fix for Wine's headers is probably
> preferable -- but I have no idea how intrusive that would be, so a
> workaround like this could be appropriate for the time being.

Okay, leaving as is.
> > > Most functions I know simply do ((PCSTR)(DWORD_PTR)ordinal) to represent
> > > ordinals (e.g. GetProcAddress). That's just a guess though, I know
> > > nothing about this function.
> >
> > I don't know that much either about this stuff -- documentation is
> > rather scarce. So the only way to find out is more tests, isn't it?
>
> That sounds right ;)  (although a step-by-step approach should be
> okay, so I don't think you have to implement this right away)

Okay, also leaving as is for now.

Thanks for your review! I've just posted an updated version [1]
including the proper ULONGs of my patch in reply to Andrew Eikum's message.

Bernhard

[1] https://www.winehq.org/pipermail/wine-devel/2014-August/105027.html

[Andrew Eikum]

I don't know this area of Wine, but some more general thoughts below.

On Tue, Aug 12, 2014 at 05:34:42PM +0200, Bernhard Reiter wrote:
> +void * WINAPI ImageDirectoryEntryToDataEx( void *base, BOOL image, unsigned short dir,
> +                                           unsigned long *size, PIMAGE_SECTION_HEADER *section );

Is there a reason for forward-declaring this instead of including
dbghelp.h?
> */
>  BOOL WINAPI BindImageEx(
> -  DWORD Flags, PCSTR ImageName, PCSTR DllPath, PCSTR SymbolPath,
> +  DWORD Flags, const char *ImageName, const char *DllPath, const char *SymbolPath,
>    PIMAGEHLP_STATUS_ROUTINE StatusRoutine)
>  {
> -  FIXME("(%d, %s, %s, %s, %p): stub\n",
> +  LOADED_IMAGE loaded_image;
> +  const IMAGE_IMPORT_DESCRIPTOR *import_desc;
> +  unsigned long size;
> +  char dll_fullname[MAX_PATH];
> +
> +  FIXME("(%d, %s, %s, %s, %p): semi-stub\n",

I know it's inconsistent with the rest of the file, but I'd use
four-space indents for this function, since it's all new code and you
have the opportunity to fix it. Not a big deal if you'd rather keep
two spaces.
> +    (*StatusRoutine)( BindImportModule, ImageName, dll_name, 0, 0);
> +

Does Windows really crash when given NULL StatusRoutine?
> +    thunk = ImageRvaToVa( loaded_image.FileHeader, loaded_image.MappedAddress,
> +                          import_desc->OriginalFirstThunk, 0 );

I don't know these functions. Is it worth checking the return value of
ImageRvaToVa? MSDN implies it can fail.
> -    todo_wine ok(status_routine_called[BindImportModule] == 1,
> -                 "StatusRoutine was called %d times\n", status_routine_called[BindImportModule]);
> +    ok(status_routine_called[BindImportModule] == 1,
> +       "StatusRoutine was called %d times\n", status_routine_called[BindImportModule]);
>
> -    todo_wine ok((status_routine_called[BindImportProcedure] == 1)
> +    ok((status_routine_called[BindImportProcedure] == 1)
>  #if defined(_WIN64)
> -                 || broken(status_routine_called[BindImportProcedure] == 0) /* < Win8 */
> +       || broken(status_routine_called[BindImportProcedure] == 0) /* < Win8 */
>  #endif
> -                 , "StatusRoutine was called %d times\n", status_routine_called[BindImportProcedure]);
> +       , "StatusRoutine was called %d times\n", status_routine_called[BindImportProcedure]);

I would avoid whitespace changes on lines you're not otherwise
changing.

Andrew

[Bernhard Reiter]

Hi Andrew,

and thanks for your helpful comments! I'm attaching an accordingly
updated version of the patch.

Am 2014-08-12 um 18:47 schrieb Andrew Eikum:
> On Tue, Aug 12, 2014 at 05:34:42PM +0200, Bernhard Reiter wrote:
> > +void * WINAPI ImageDirectoryEntryToDataEx( void *base, BOOL image, unsigned short dir,
> > +                                           unsigned long *size, PIMAGE_SECTION_HEADER *section );
>
> Is there a reason for forward-declaring this instead of including
> dbghelp.h?

See my answer [1] to Thomas Faber's message [2]:

Am 2014-08-12 um 19:34 schrieb Bernhard Reiter:
> Including dbghelp.h along with imagehlp.h unfortunately gives a host of
> definition collisions. From MSDN I know that some of dbghelp's functions
> are mirrored in imagehlp, but apparently something's wrong here in Wine...

Am 2014-08-12 um 18:47 schrieb Andrew Eikum:
> I know it's inconsistent with the rest of the file, but I'd use
> four-space indents for this function, since it's all new code and you
> have the opportunity to fix it. Not a big deal if you'd rather keep
> two spaces.

Okay, got me convinced.
> > +    (*StatusRoutine)( BindImportModule, ImageName, dll_name, 0, 0);
>
> Does Windows really crash when given NULL StatusRoutine?

You're right, it doesn't, as an additional test [2] reveals. Added logic
to catch that.
> > +    thunk = ImageRvaToVa( loaded_image.FileHeader, loaded_image.MappedAddress,
> > +                          import_desc->OriginalFirstThunk, 0 );
>
> I don't know these functions. Is it worth checking the return value of
> ImageRvaToVa? MSDN implies it can fail.

Added logic to catch and bail out.
> I would avoid whitespace changes on lines you're not otherwise
> changing.

That's the one thing I'd like to keep, as they're always following lines
that changed (where todo_wine is removed) (modulo macros).

Bernhard

[1] https://www.winehq.org/pipermail/wine-devel/2014-August/104998.html
[2] https://www.winehq.org/pipermail/wine-devel/2014-August/104996.html
[3] http://source.winehq.org/patches/data/106065