[PATCH] msdmo: Fix null deref in any_types_match.

[Zebediah Figura]

On 9/30/20 11:58 AM, Patrick Hibbs wrote:
> 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?
> 

It's a tradeoff, I guess, between zealously assert()ing every time
something should be true, and never doing so, and I think in this case
the assert just wouldn't be worthwhile. It's a common enough pattern
that some_array should contain valid space for at least some_array_count
elements, that I think the code is pretty self-explanatory as it is, and
I get the impression usual Wine code agrees with that level of robustness.

> -- Patrick
> 
>
>

ἔρρωσο,
Zebediah

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200930/f5efa079/attachment-0001.sig>