Re: [ntdll] fix GetProcAddress on mapped modules as data

[Vitaliy Margolen]

Monday, November 28, 2005, 3:29:51 PM, Raphael wrote:
> Hi,
> fix crash on Corel 9 start
> (http://bugs.winehq.org/show_bug.cgi?id=2884)
> Changelog:
> - don't crash on GetProcAddress when input mapped modules as data

I think you need to make some tests of this. It shouldn't be to hard to
load some dll as a resource and try to get proc address. Also check if
you can do that for already loaded dll (like kernel32.dll).

Now the patch:
> - if ((ULONG_PTR)module & 1)
> - {
> -
>    module = (HMODULE)((ULONG_PTR)module & ~1);
> - }

Check is some-what redundant. Just use:
module = (HMODULE)((ULONG_PTR)module & ~1);
> -
>    void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1 )
> -
>                      : find_ordinal_export( module, exports, exp_size, ord - exports->Base );
> -
>    void *proc = NULL;
> -
>   if (name) 
> -
>   { 
> -
>        proc = find_named_export( module, exports, exp_size, name->Buffer, -1 );
> -
>   }
> -
>    else
> -
>    {
> -
>        proc = find_ordinal_export( module, exports, exp_size, ord - exports->Base );
> -
>   }

Any reason for this? It's the same no matter how you write it only 7
lines longer <g>

Vitaliy Margolen

[ntdll] fix GetProcAddress on mapped modules as data

[Raphael]

On Tuesday 29 November 2005 00:03, Vitaliy Margolen wrote:
> Monday, November 28, 2005, 3:29:51 PM, Raphael wrote:
> > Hi,
> > fix crash on Corel 9 start
> > (http://bugs.winehq.org/show_bug.cgi?id=2884)
> > Changelog:
> > - don't crash on GetProcAddress when input mapped modules as data
>
> I think you need to make some tests of this. It shouldn't be to hard to
> load some dll as a resource and try to get proc address. Also check if
> you can do that for already loaded dll (like kernel32.dll).

I don't have a windows install to test.
> Now the patch:
> > - if ((ULONG_PTR)module & 1)
> > - {
> > -
> >    module = (HMODULE)((ULONG_PTR)module & ~1);
> > - }
>
> Check is some-what redundant. Just use:
> module = (HMODULE)((ULONG_PTR)module & ~1);

I use the same (and more readable) test as RtlImageDirectoryEntryToData
> > -
> >    void *proc = name ? find_named_export( module, exports,
> >
> > exp_size, name->Buffer, -1 ) -                          :
> > find_ordinal_export( module, exports, exp_size, ord - exports->Base ); + 
> >       void *proc = NULL;
> > -
> >   if (name)
> > -
> >   {
> > -
> >        proc = find_named_export( module, exports, exp_size,
> >
> > name->Buffer, -1 ); +       }
> > -
> >    else
> > -
> >    {
> > -
> >        proc = find_ordinal_export( module, exports, exp_size, ord -
> >
> > exports->Base ); +       }
>
> Any reason for this? It's the same no matter how you write it only 7
> lines longer <g>

Because i found it more readable.
using call functions in ternary operator make code unreadable.
> Vitaliy Margolen

Regards,
Raphael

[ntdll] fix GetProcAddress on mapped modules as data

[Vitaliy Margolen]

Monday, November 28, 2005, 4:34:08 PM, Raphael wrote:
> On Tuesday 29 November 2005 00:03, Vitaliy Margolen wrote:
> > Monday, November 28, 2005, 3:29:51 PM, Raphael wrote:
> > > Hi,
> > > fix crash on Corel 9 start
> > > (http://bugs.winehq.org/show_bug.cgi?id=2884)
> > > Changelog:
> > > - don't crash on GetProcAddress when input mapped modules as data
> >
> > I think you need to make some tests of this. It shouldn't be to hard to
> > load some dll as a resource and try to get proc address. Also check if
> > you can do that for already loaded dll (like kernel32.dll).
>
> I don't have a windows install to test.

Still you need to make tests and send them first then
> > Now the patch:
> > > - if ((ULONG_PTR)module & 1)
> > > - {
> > > -
> > >    module = (HMODULE)((ULONG_PTR)module & ~1);
> > > - }
> >
> > Check is some-what redundant. Just use:
> > module = (HMODULE)((ULONG_PTR)module & ~1);
>
> I use the same (and more readable) test as RtlImageDirectoryEntryToData

What to test? If you resetting that bit, when it's set, and not resetting
it when it's not set result is the same. So dropping the test will make
it more clear actually.
> > > -
> > >    void *proc = name ? find_named_export( module, exports,
> > >
> > > exp_size, name->Buffer, -1 ) -                          :
> > > find_ordinal_export( module, exports, exp_size, ord - exports->Base ); +
> > >       void *proc = NULL;
> > > -
> > >   if (name)
> > > -
> > >   {
> > > -
> > >        proc = find_named_export( module, exports, exp_size,
> > >
> > > name->Buffer, -1 ); +       }
> > > -
> > >    else
> > > -
> > >    {
> > > -
> > >        proc = find_ordinal_export( module, exports, exp_size, ord -
> > >
> > > exports->Base ); +       }
> >
> > Any reason for this? It's the same no matter how you write it only 7
> > lines longer <g>
>
> Because i found it more readable.
> using call functions in ternary operator make code unreadable.

Well that depends <g> But shouldn't it be a separate patch? As it has
nothing to do with the change-log?

Vitaliy