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

graphics-hook: Fix null pointer dereference #11430

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

Conversation

Charlese2
Copy link

Multiple APIs may be setup to capture without being initialized in graphics-hook when multiple threads are sending present calls. This just prevents those invalid captures from completing.

Description

Adds a check for data.handle being null, as that currently is one indicator that the data struct has not been initialized.

Motivation and Context

In the NVIDIA 555.85 driver, an extra API queue was added when using VK_PRESENT_MODE_FIFO_KHR or VK_PRESENT_MODE_FIFO_RELAXED_KHR present modes. Two separate threads present to each of the queues. The application presents to the Vulkan queue, and the driver presents to the DirectX 12 queue (DXGI). The user-mode driver copies the contents of the vulkan framebuffer into in the the DirectX 12 queue. After that the DX12 queue is the one that is rendered to the window.

Fixes #11403

How Has This Been Tested?

Tested under DX12, DX11, OpenGL, and Vulkan to make sure they still capture. (using Dolphin Emulator video backends)
Tested disabling and re-enabling an active capture while using Vulkan and VSync to make sure it doesn't crash.

On Windows 11 23H2 using a NVIDIA GeForce 3080.

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.

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.

Yeah, for the time being this fix might be unavoidable considering that it will consider the capture active in the other API as well. This is kind of annoying, and I wish we could somehow confine this check specifically inside capture_ready() rather than have to add another part to each expression where it's checked.

Could you put the null check before capture_ready() in each of these though? Unfortunately capture_ready() may also affect the frame time timer, so it's better to do the null pointer check before calling it. Then I'll give my approval.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Windows Affects Windows labels Oct 26, 2024
Multiple APIs may be setup to capture without being initialized in graphics-hook when multiple threads are sending present calls. This just prevents those invalid captures from completing.
@Charlese2 Charlese2 force-pushed the fix-null-pointer-dereference-crash branch from 5460cca to 7b01a6d Compare October 27, 2024 01:14
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 Windows Affects Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

graphics-hook crashing Vulkan applications while VSync is enabled.
3 participants