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

[RytoEX]

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:

• libobs/media-io: Avoid allocating frame memory if size is zero #11803

Fixes #11729
Fixes #11860

How Has This Been Tested?

Tested locally by forcing videoConfig.cx or videoConfig.cy_abs 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.

[notr1ch]

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

[RytoEX]

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]

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]

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.

[RytoEX]

Per off-thread discussion, moved the zero checks from DShowInput::OnVideoData to DShowInput::UpdateVideoConfig.