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.

Sign up for GitHub

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

Jump to bottom

win-dshow: Fix possible crash if frame width or height is zero #11861

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

win-dshow: Fix possible crash if frame width or height is zero #11861

wants to merge 1 commit into from
+6 −0

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Feb 13, 2025

Description

If a frame has a width or height of zero, this value will make it into libobs/media-io/video-frame.c:video_frame_init and cause linesizes or heights to be zero, which will result in a bmalloc(0) call and OBS will crash.

Instead of letting the call stack get that far, check the frame width and height here at the source, log an error, and return early if the frame width or height are zero.

Motivation and Context

Alternative to:

Fixes #11729
Fixes #11860

How Has This Been Tested?

Tested locally by forcing frame.width or frame.height above this change to be zero and verifying that the error is logged and OBS does not crash.

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.

@RytoEX
win-dshow: Fix possible crash if frame width or height is zero
e1385c6 
If a frame has a width or height of zero, this value will make it into
libobs/media-io/video-frame.c:video_frame_init and cause linesizes or
heights to be zero, which will result in a bmalloc(0) call and OBS will
crash.

Instead of letting the call stack get that far, check the frame width
and height here at the source, log an error, and return early if the
frame width or height are zero.
@RytoEX RytoEX added this to the OBS Studio 31.0 milestone Feb 13, 2025
@RytoEX RytoEX requested a review from tt2468 February 13, 2025 20:17
@RytoEX RytoEX self-assigned this Feb 13, 2025
@notr1ch
Copy link
Member

notr1ch commented Feb 13, 2025

Wouldn't this imply the numbers in VideoConfig are bad? Perhaps it would be better to fix it there?

@RytoEX
Copy link
Member Author

RytoEX commented Feb 13, 2025 •
edited
Loading

Wouldn't this imply the numbers in VideoConfig are bad? Perhaps it would be better to fix it there?

Possibly? I was told we should check size for zero in video_frame_init and made #11803. There I was told that's not sufficient, and it was later suggested to instead check frame size in win-dshow before the obs_source_output_video2 call, so I made this. I want to ship a crash fix since we intentionally made bmalloc(0) crash in OBS Studio 31.

We could try to move this check to DShowInput::UpdateVideoConfig where DShowInput::videoConfig is populated, but the only log I have seen where this happens (in #11729) does not show that the device was set to have a zero width or height. If OnVideoData is being called (as videoConfig.callback) before any of the logging happens, then it's possible the normal logs would not show anything to indicate that this is about to crash/fail.

We could check around here:

obs-studio/plugins/win-dshow/win-dshow.cpp

Lines 915 to 924 in 80ea1b1

videoConfig.name = id.name;
videoConfig.path = id.path;
videoConfig.useDefaultConfig = resType == ResType_Preferred;
videoConfig.cx = cx;
videoConfig.cy_abs = abs(cy);
videoConfig.cy_flip = cy < 0;
videoConfig.frameInterval = interval;
videoConfig.internalFormat = format;

However, cx and cy are initialized to 0 above that, and will not be changed if resType is not ResType_Custom, so that doesn't seem correct.

We could check after SetVideoConfig:

obs-studio/plugins/win-dshow/win-dshow.cpp

Lines 934 to 938 in 80ea1b1

if (!device.SetVideoConfig(&videoConfig)) {
blog(LOG_WARNING, "%s: device.SetVideoConfig failed", obs_source_get_name(source));
return false;
}

Perhaps this?

@@ -942,6 +936,12 @@ bool DShowInput::UpdateVideoConfig(obs_data_t *settings)
                return false;
        }

+       if (!videoConfig.cx || !videoConfig.cy_abs) {
+               blog(LOG_ERROR, "%s: Frame width or height are zero (%" PRIu32 "x%" PRIu32 ")",
+                    obs_source_get_name(source), videoConfig.cx, videoConfig.cy_abs);
+               return false;
+       }
+

This would be after libdshowcapture sets the device config (via HDevice::SetVideoConfig() -> HDevice::SetupVideoCapture() -> HDevice::ConvertVideoSettings()), but I'm not sure if OnVideoData would be called separately.

Happy to get some guidance here on where a better place to check the frame/config width and height would be.

@Lain-B
Copy link
Collaborator

Lain-B commented Feb 14, 2025

It's probably not a huge deal if it's not in the right place and this ends up being a redundant check. If there were specific steps to reproduce that'd be ideal but this is sufficient for now if it's not trivial to reproduce.

Lain-B
Lain-B approved these changes Feb 18, 2025
View reviewed changes
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.

Even if it's not the perfectly right place, if this fixes it for now it's fine -- if there's a better solution we can add that fix in addition later.

@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lain-B Lain-B Lain-B approved these changes

@tt2468 tt2468 Awaiting requested review from tt2468

Assignees

@RytoEX RytoEX

Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Milestone
OBS Studio 31.0
4 participants
@RytoEX @notr1ch @Lain-B @WizardCM