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

libobs/util: Crash on bmalloc(0) #11181

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

Conversation

gxalpha
Copy link
Member

@gxalpha gxalpha commented Aug 23, 2024

Description

As outlined in c5965c8 (#6721), bmalloc(0) is pretty much always a mistake, possibly hiding other bugs. It's been two years since that commit introduced a warning announcing that this will crash in a future version of OBS, let's make that happen.

cc @notr1ch

Motivation and Context

People are breaking stuff (see other PRs), let's break everything at once.
This gives plugin developers who might (unknowingly) do this the opportunity to fix it.

If there are places where we're doing it in OBS itself, this is a great (and certainly loud) way to find out.

How Has This Been Tested?

macOS 15
Launched OBS and played around, did not observe crashes.

Manually added a malloc(0) call, had OBS crash as expected.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

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 requested a review from notr1ch August 23, 2024 21:36
@kkartaltepe
Copy link
Collaborator

this is typically reported in pipewire captures, can we fix that first?

@gxalpha
Copy link
Member Author

gxalpha commented Aug 24, 2024

To clarify, the "please fix your code! This will crash in future versions of OBS" message currently appears in normal operation using pipewire? That arguably should have been fixed during the last two years, so yes that should be fixed; but in my opinion it shouldn't necessarily hold up this PR - the apparent evidence from the last two years would suggest that it could take ages (seeing that "this will crash in future versions" isn't enough motivation).

@WizardCM WizardCM added the Enhancement Improvement to existing functionality label Aug 24, 2024
@tytan652
Copy link
Collaborator

tytan652 commented Aug 25, 2024

params = bzalloc(2 * obs_pw_stream->format_info.num *
sizeof(struct spa_pod *));
is the only place where it can happen.

@RytoEX
Copy link
Member

RytoEX commented Aug 29, 2024

params = bzalloc(2 * obs_pw_stream->format_info.num *
sizeof(struct spa_pod *));

is the only place where it can happen.

I guess the question is whether or not obs_pw_stream->format_info.num is expected to be 0 at times, and if so, should we be calling bzalloc when it is?

@PatTheMav
Copy link
Member

params = bzalloc(2 * obs_pw_stream->format_info.num *
sizeof(struct spa_pod *));

is the only place where it can happen.

I guess the question is whether or not obs_pw_stream->format_info.num is expected to be 0 at times, and if so, should we be calling bzalloc when it is?

Even if it is expected to be zero at times then it needs to be checked and an allocation only take place if is has a >0 value.

As linux-pipewire is an internal module, it needs to be updated in tandem with the change this PR makes, so I'd just add the check and let other "bad" allocations that happen at runtime duly crash the program.

@RytoEX
Copy link
Member

RytoEX commented Sep 12, 2024

I guess the question is whether or not obs_pw_stream->format_info.num is expected to be 0 at times, and if so, should we be calling bzalloc when it is?

Even if it is expected to be zero at times then it needs to be checked and an allocation only take place if is has a >0 value.

As linux-pipewire is an internal module, it needs to be updated in tandem with the change this PR makes, so I'd just add the check and let other "bad" allocations that happen at runtime duly crash the program.

My question was poorly phrased. It should have been "should we actually be calling bzalloc when it is?" or rephrased as a statement such as, "if so, then we shouldn't be calling bzalloc when it is 0."

While I welcome a change to linux-pipewire, I would think that change is out of scope for this PR and would require the expertise of someone who works with the linux-pipewire code, such as @GeorgesStavracas , @kkartaltepe , or @tytan652 .

As outlined in c5965c8, bmalloc(0) is
pretty much always a mistake, possibly hiding other bugs.
It's been two years since that commit introduced a warning announcing
that this will crash in a future version of OBS, let's make that happen.
@gxalpha gxalpha force-pushed the crashes-are-good-questionmark branch from a1abf08 to ba497df Compare October 5, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants