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

frontend/widgets: Fix integer overflow #11922

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

Conversation

Lain-B
Copy link
Collaborator

@Lain-B Lain-B commented Mar 5, 2025

Description

If the crop values combined are larger than the width or height of the source, an integer overflow will occur.
This fix converts the width/height values to int, and then clamps any negative values to 0.

Fixes #11917

Motivation and Context

This was one of the root causes behind the freeze reported in #11917. Due to the integer overflow, it would essentially make the for loop in DrawStripedLine() in OBSBasicPreview.cpp repeat nearly endlessly, which caused certain mutexes to stall (especially the scene video/audio mutexes). Because of that near-infinite loop, both the main and audio threads would stall until that near-indefinitely-stalled loop completed.

How Has This Been Tested?

Tested according to the reproduction steps in #11917 and it no longer stalls.

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.

If the crop values combined are larger than the width or height of the
source, an integer overflow will occur.

This fix converts the width/height values to int, and then clamps
any negative values to 0.
@exeldro
Copy link
Contributor

exeldro commented Mar 5, 2025

The check is done after it is multiplied with scale, but scale is allowed to be negative for flipped sources.
So the multiplying by scale should be moved after the check.

BTW my alternate solution to fix the same issue is in #11919 and my fix for already extreme large sources in #11920

@@ -419,8 +419,15 @@ static vec2 GetItemSize(obs_sceneitem_t *item)

obs_sceneitem_get_scale(item, &scale);
obs_sceneitem_get_crop(item, &crop);
size.x = float(obs_source_get_width(source) - crop.left - crop.right) * scale.x;
size.y = float(obs_source_get_height(source) - crop.top - crop.bottom) * scale.y;
size.x = float((int)obs_source_get_width(source) - crop.left - crop.right) * scale.x;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the multiplying by scale should be moved after the check, because scale can be negative for flipped sources

@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Mar 5, 2025
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.

Use crop, drag source, ui block occurs
3 participants