Skip to content

Conversation

@RytoEX
Copy link
Member

@RytoEX RytoEX commented Jan 31, 2025

Description

It is possible that size can still be zero after the preceding loop. If that happens, and bmalloc is called with 0 as its argument, OBS will crash as intended with OBS Studio 31. Avoid crashing by not allocating memory for a frame with a size of zero.

Motivation and Context

See the crash in:

If there's a better place to do this check, I'm open to alternatives.

How Has This Been Tested?

Compiled and ran locally on Windows 11 with a Video Capture Device added.

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.

It is possible that size can still be zero after the preceding loop. If
that happens, and bmalloc is called with 0 as its argument, OBS will
crash as intended with OBS Studio 31. Avoid crashing by not allocating
memory for a frame with a size of zero.
@RytoEX RytoEX added the Bug Fix Non-breaking change which fixes an issue label Jan 31, 2025
@RytoEX RytoEX added this to the OBS Studio 31.0 milestone Jan 31, 2025
@RytoEX RytoEX requested a review from tt2468 January 31, 2025 19:24
@RytoEX RytoEX self-assigned this Jan 31, 2025
@tt2468
Copy link
Member

tt2468 commented Jan 31, 2025

I think my primary worry here would be that video_frame_init is a void return, so we might not actually be fixing any issues, rather pushing the undefined behavior to somewhere else.

@RytoEX
Copy link
Member Author

RytoEX commented Jan 31, 2025

I think my primary worry here would be that video_frame_init is a void return, so we might not actually be fixing any issues, rather pushing the undefined behavior to somewhere else.

video_frame_init can already early return:

if (!frame)
return;

That said, I'm open to alternatives, but we intentionally crash if bmalloc(0) is called now, so we should really try to avoid calling bmalloc(0).

@Lain-B
Copy link
Collaborator

Lain-B commented Feb 2, 2025

We could add video_frame_init2() that returns a bool. Failures should probably be handled rather than implicitly ignored. If I did that (I'm assuming I did) then that was my mistake, but I think that it would also be nice that whatever is causing it to get a size of 0 should probably also be fixed as well, however that's happening.

@RytoEX
Copy link
Member Author

RytoEX commented Feb 4, 2025

We could add video_frame_init2() that returns a bool. Failures should probably be handled rather than implicitly ignored. If I did that (I'm assuming I did) then that was my mistake, but I think that it would also be nice that whatever is causing it to get a size of 0 should probably also be fixed as well, however that's happening.

video_frame_init has been void before and since the rewrite of this file. It has had at least one early return for almost 11 years.

The crash that motivated this PR is mentioned in #11729. Some issue with a Video Capture Device source on Windows caused OBS to crash when bmalloc(0) was called (as we designed it to do as of OBS Studio 31).

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.

3 participants