[PATCH] msdmo: Fix null deref in any_types_match.

[Patrick Hibbs]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Wed, 2020-09-30 at 10:55 -0500, Zebediah Figura wrote:
> Hello Patrick,
> 
> On 9/30/20 8:43 AM, Patrick Hibbs wrote:
> > Signed-off-by: Patrick Hibbs <hibbsncc1701 at gmail.com>
> > ---
> >  dlls/msdmo/dmoreg.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/dlls/msdmo/dmoreg.c b/dlls/msdmo/dmoreg.c
> > index 8e0680931f4..42e4eab3651 100644
> > --- a/dlls/msdmo/dmoreg.c
> > +++ b/dlls/msdmo/dmoreg.c
> > @@ -451,10 +451,16 @@ static BOOL any_types_match(const
> > DMO_PARTIAL_MEDIATYPE *a, unsigned int a_count
> >  
> >      for (i = 0; i < a_count; ++i)
> >      {
> > -        for (j = 0; j < b_count; ++j)
> > +        if (a != NULL)
> >          {
> > -            if (IsMediaTypeEqual(&a[i], &b[j]))
> > -                return TRUE;
> > +            for (j = 0; j < b_count; ++j)
> > +            {
> > +                if (b != NULL)
> > +                {
> > +                    if (IsMediaTypeEqual(&a[i], &b[j]))
> > +                        return TRUE;
> > +                }
> > +            }
> >          }
> >      }
> >      return FALSE;
> > 
> 
> This seems like the wrong solution; "types" shouldn't be NULL if
> "size"
> is nonzero. That it is is a bug in itself, which I diagnosed in [1]
> but
> never got around to sending a patch for...
> 
> [1] https://bugs.winehq.org/show_bug.cgi?id=49659
> 
Hello Zebediah, 

Yeah, that's the same bug.

However, I still think the patch should be looked at. (Although I did
just notice a bit of redundancy on my part. That check should only be
done once per function call.)

Wine will crash whatever thread winds up calling this function if the a
and / or b argument is set to NULL. If that's not supposed to happen
when "size" > 0, then it should be checked for and enforced by the code
before any derefs occur.

As the deref occurs in this function it makes sense to check for and
enforce the expectations here. 

As it stands, wine crashes because it violated it's own assumptions. In
my case, fixing the issue by enforcing the assumption allowed another
type to deal with the media stream. Which allowed the program I was
debugging to work as intended. I think that would be a better potential
outcome for wine's users than wine crashing abruptly.

What do you think?

- -- Patrick

-----BEGIN PGP SIGNATURE-----

iQGzBAEBCAAdFiEE0xN9TKR2ED88setNaE6EVLZHukoFAl90uTcACgkQaE6EVLZH
ukqGFQwAh/imZcTSZqt8WM9wrD+P78YfCJj1NSd0P47O4FR8fdCp0cXbrr9aKgdS
34QUIsFV5rAysGALbbIK/pun1aqEEHFD/jlm1Jfwjd5kN61Qg1JT/ZnLgb5ZDSCX
pq22Lc26T+pdUskYY3bl2aFINnhFASRwNw/OcHWdTfURYmH7khM0k6iSd2QFXCC6
HRMPoZv+nTjt+CyRttKrE6rDjcBgfvaM51xQLFJnBlBOr+Uyha1SFDMRlXLiHeKk
8vnDBPdKHuSE3EBOuuAO68WWjSa71WDaXJvYjki6WbICVM80s2NtQbc57/NgqQSh
0u2N4hBohrZtLRu7TD/a9XoiUeMpbFu6J5naPQqSM28dG0WvxZ2XywnGkPwL4h/4
bVpLDO89Xw6tm1t82rhQvilDKFCHh9SLuIaHYGrszLuQf5m4clVDQqU+ZqFsAT2J
yhHosxr39pmfjEO04XWfC7fbWPMI/6icuFN47Iz4giD3RECF8P6x+v0yA87I9CXP
4IyAhbQS
=EojX
-----END PGP SIGNATURE-----