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

win-capture: Avoid NULL deref when capture not initialized #11375

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

confusionattack
Copy link

Description

Add a check to avoid a NULL dereference on backbuffer capture in the D3D10, D3D11, and D3D12 capture modules in case d3d10_data, d3d11_data, or d3d12_data has not yet been successfully configured. This is similar to NULL checks in other places in these modules, such as the d3d10_free, d3d11_free, and d3d12_free functions. This change is intended to fix a specific issue with D3D12 capture we are seeing the wild, but for completeness we added NULL checks to the D3D10 and D3D11 capture modules.

Motivation and Context

We were seeing an intermittent crash in graphics-hook when using OBS Studio to capture the Midnight Murder Club demo (https://store.steampowered.com/app/2698870/Midnight_Murder_Club/) on some laptops with multiple GPUs. We tracked it down to a NULL dereference when capturing the backbuffer in the D3D12 capture module.

How Has This Been Tested?

We were able to a user who could reliably replicate the crash on an Intel i7-11800H laptop running Windows 11 with an NVIDIA GeForce RTX 3060 Laptop GPU and a built-in Intel UHD GPU. The repro steps were as follows:

  • Start the MMC demo and progress the main menu.
  • Create two scenes in OBS Studio. The first scene is a static image. The second scene is the game window capture.
  • Rapidly toggle between the two scenes until a crash occurs.

After applying implementing the NULL check fix and testing against a locally built OBS Studio, we were unable to reproduce any issue.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@gxalpha gxalpha added the Bug Fix Non-breaking change which fixes an issue label Oct 9, 2024
@derrod derrod requested review from jpark37 and Lain-B October 9, 2024 17:51
@jpark37 jpark37 removed their request for review October 10, 2024 14:49
@jpark37
Copy link
Collaborator

jpark37 commented Oct 10, 2024

Removed myself as a reviewer since I'm not active enough to be holding things up.
Thanks for the PR. I suspect my preference would be to resolve the condition leading this implied invariant to break rather than sprinkle around null checks. Even if that fix means sprinkling a null check somewhere else. :P Something about our understand of how the code can flow is probably flawed here, so it would be good to try to understand how this is happening before making changes. Not asking you to do the digging, but someone should.
I'll stay out of this from here and let Lain handle it.

@confusionattack
Copy link
Author

I take jpark37's points. This change does not really get at root causes. It's little challenging on my end because I don't have physical access to any of the machines where this crash is being reported. But with more effort, I can determine the root issue on the graphics-hook side. Or maybe I should just focus on modifying the game client to avoid putting graphics-hook in a position where it crashes and then this PR won't be needed at all? Lain-B, do you have any thoughts?

@WizardCM WizardCM self-requested a review October 26, 2024 23:15
@RytoEX
Copy link
Member

RytoEX commented Nov 1, 2024

For reference, I believe #11430 goes the "sprinkling a null check somewhere else" route by checking data.handle before trying to call any of the _capture functions.

@confusionattack does that PR (already merged) resolve your observed crashes?

Copy link
Collaborator

@Lain-B Lain-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this happens because two APIs can be used at the same time, so capture_active() will return true even in other hooked APIs, which results in this issue. I think for the time being this is an acceptable change and works fine to alleviate this issue.

I'm just going to force-push a change to rebase and increment the graphics hook patch version on this PR's branch and then we can merge this.

Regardless, this will not cause a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants