[MPlayer-dev-eng] [PATCH] CoreAVC [3/5] - Win32

[Alan Nisota]

On 10/5/06, Reimar Döffinge wrote:
> Hmm.. why is the signal handler needed? What irritates me is that it is
> a handler for SIGSEGV... And I'm not sure if it's not a bit dangerous
> security-wise to jump to locations on stack on a segfault (that might
> also happen e.g. as side-effect of a malloc failing, though since it's
> only for DLL entry functions it's not so likely to be critical).
> Isn't it maybe possible to just patch away the offending instruction?
I think this is a part of the PECompact2 code.  What it does is to
modify the code (in memory) on the fly.  I believe it then uses a
sigsegv (which is caught) to jump into the new code.  I could probably
get you a gdb trace of what is going on.  This was a pain to make work
as I recall.

>
> > +        if(name && strcasecmp(name, "oleaut32")==0 || !strcasecmp(name, "oleaut32.dll"))
> > +           result=MODULE_HANDLE_oleaut32;
>
> Since && has higher priority that || I don't think this does what you
> meant.
> Also since the surrounding code does you should use tabs here instead of
> spaces.
right.  will fix.

> [expVirtualProtect]
I'll add a TODO here

> > @@ -795,7 +807,8 @@
> >         return (void*)WAIT_FAILED;
> >      }
> >      dbgprintf("WaitForSingleObject(0x%x, duration %d) =>\n",object, duration);
> > -
> > +    if ((int)ml == 1)
> > +       return 0;
>
> Why 0 and not (void*)WAIT_FAILED or WAIT_OBJECT_0 some other constant like all other
> cases return?
Wil fix

> > +#else
> > +    return 1;
> >  #endif
> >  }
>
> Hmm.. is this related to the previous one? Does return 0 also work,
> because that case is already handled in expWaitForSingleObject anyway...
No I believe 1 (or at least >0) is expected by the code.  I'll reverify though.

> > +static void * WINAPI expDecodePointer(void *value)
> > +{
> > +    return (void *)((long)value ^ getpid());
> > +}
>
> IMHO these should be NOPs (i.e. return the same value as they were
> given). There is no "security" anyway when using binary codecs.
That is how I had it in the first patch.  Someone on list asked me to
change it to this way.  Make up your mind :)

> > +static int WINAPI expLoadStringW(long instance, long  id, void* buf, long size)
> > +{
> > +    return expLoadStringA(instance, id, buf, size);
> > +}
>
> does
> #define expLoadStringW expLoadStringA
> work as well?
> Or actually just add
> {"LoadStringW", -1, (void*)expLoadStringA},
I don't see why not, I'll change and verify

> > [expGetVersionExA changes]
> not sure if they might not break something. Some codecs might try to use
> more "enhanced" features we have not yet implemented when we increase
> the reported version number. Can you investigate why they are necessary?
The codec explicity checks for these values, and fails (as I
recall)without them set to at least these.

> What does a codec need the Desktop Window for?? And is this change
> related to the WaitForSingleObject one or not?
I don't think it is related (I'll double check).  I think this has to
do with initializing the options menus for the codec (even though we
never use it).  It was certainly needed though.  I could be wrong, it
could have to do with the codec having direct-display capabilities
(which again we don't use).  I don't know why, but I couldn't get it
to load without this set.

>
> > +#define UNDEFF(X, Y) \
> > +    {#X, Y, -1},
>
> Huh? Why -1 and then check for -1 in LookupExternalByName instead of
> just using 0? Also what is the problem with the current code that would
> just call do return add_stub(); for this case?
The add_stub code makes the function available, while this does not.
If the function is available, then the codec assumes it can use it,
and this causes issues.  The whole point was to define some functions
which get stubs, and others which return 'not available'.  there
doesn't appear to be a mechanism for this in the current code.

Thanks for the feedback.