[PATCH] msdmo: Fix null deref in any_types_match.
Signed-off-by: Patrick Hibbs <hibbsncc1701(a)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; -- 2.28.0
Hello Patrick, On 9/30/20 8:43 AM, Patrick Hibbs wrote:
Signed-off-by: Patrick Hibbs <hibbsncc1701(a)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
-----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(a)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...
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-----
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(a)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...
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
participants (2)
-
Patrick Hibbs -
Zebediah Figura