Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VAAPI: fix use after free in CVAAPIContext::DestroyContext #16763

Merged
merged 1 commit into from Oct 27, 2019

Conversation

anssih
Copy link
Member

@anssih anssih commented Oct 13, 2019

Commit cfd5959 ("VAAPI: add error and info callbacks") added calls
into vaSetInfoCallback() and vaSetErrorCallback() in
CVAAPIContext::DestroyContext().

However, those calls are done after vaTerminate(m_display) which frees
the context, causing use after free.

Fix that by only performing the callback removal if vaTerminate() fails.

Also, set m_display to NULL after a successful vaTerminate() to avoid
similar issues being hidden in the future.

Commit cfd5959 ("VAAPI: add error and info callbacks") added calls
into vaSetInfoCallback() and vaSetErrorCallback() in
CVAAPIContext::DestroyContext().

However, those calls are done after vaTerminate(m_display) which frees
the context, causing use after free.

Fix that by only performing the callback removal if vaTerminate() fails.

Also, set m_display to NULL after a successful vaTerminate() to avoid
similar issues being hidden in the future.
@anssih anssih added Type: Fix non-breaking change which fixes an issue Component: Video v19 Matrix labels Oct 13, 2019
@anssih anssih requested a review from lrusak October 13, 2019 07:36
@fritsch
Copy link
Member

fritsch commented Oct 13, 2019

Thanks much.

I am not a friend of the "indent" when using #ifdef - but there are arguments for it (debugging code where it is evaluated to true) and against.

@lrusak
Copy link
Contributor

lrusak commented Oct 23, 2019

should these callbacks be done before vaTerminate instead?

@anssih
Copy link
Member Author

anssih commented Oct 23, 2019

I think that would work as well.

I believe it is only a cosmetic difference as the context the callbacks are stored in is destroyed anyway. But I guess I could be wrong :) And we would avoid the additional conditional.

@lrusak
Copy link
Contributor

lrusak commented Oct 23, 2019

I think that would work as well.

I believe it is only a cosmetic difference as the context the callbacks are stored in is destroyed anyway. But I guess I could be wrong :) And we would avoid the additional conditional.

It's up to you. I'm fine either way. I think I did it this way originally because I wanted to catch any issues with vaTerminate in the log (I'm not sure if there is anything logged in that call though).

@anssih
Copy link
Member Author

anssih commented Oct 27, 2019

I think that would work as well.
I believe it is only a cosmetic difference as the context the callbacks are stored in is destroyed anyway. But I guess I could be wrong :) And we would avoid the additional conditional.

It's up to you. I'm fine either way. I think I did it this way originally because I wanted to catch any issues with vaTerminate in the log (I'm not sure if there is anything logged in that call though).

Ah, good point, that's probably why I kept the calling order. So I do prefer it this way after all :)

I also doubt vaTerminate() ever logs anything, but that depends on the backend as well so one could make a backend that does log.

@lrusak lrusak merged commit eb2a704 into xbmc:master Oct 27, 2019
@lrusak lrusak added this to the Matrix 19.0-alpha 1 milestone Oct 27, 2019
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020
VAAPI: fix use after free in CVAAPIContext::DestroyContext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants