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

UI: Address logging buffer size discrepancies #10787

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

jcm93
Copy link
Contributor

@jcm93 jcm93 commented Jun 6, 2024

Description

Changes the remaining log buffer allocations in the log handler to use a buffer of size 8192 instead of 4096. Also adds improved string handling in the form of using strncpy rather than strcpy in the too_many_repeated_entries() check.

Motivation and Context

OBS was crashing on startup when logging messages over 4096 characters in length with corruption of the QPointer object for the log viewer. It turns out that this was due to a buffer overrun in the log handling code.

43a1b30 changed the size of the log buffer in obs-app.cpp's do_log, but to change the size of the buffer properly we need to also modify the buffer size in the subsequent check for repeated entries, as well as the base log handler in libobs. Otherwise the repeated entries check will bulldoze over memory adjacent to cmp_str if fed a message exceeding 4096 characters in length, as well as other memory in the base log handler.

How Has This Been Tested?

Verified that OBS no longer crashes when failing to load VLC on startup, and logging continues to work correctly.

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 RytoEX requested review from derrod and PatTheMav June 6, 2024 20:43
@RytoEX RytoEX self-assigned this Jun 6, 2024
@RytoEX RytoEX added the Bug Fix Non-breaking change which fixes an issue label Jun 6, 2024
@RytoEX RytoEX added this to the OBS Studio (Next Version) milestone Jun 6, 2024
@RytoEX
Copy link
Member

RytoEX commented Jun 6, 2024

cc @palana who authored 43a1b30 in #10633.

Copy link
Contributor

@palana palana left a comment

Choose a reason for hiding this comment

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

looks good to me 👍 the only suggestion I'd have beyond this is to move the buffer size into a constant somewhere to avoid this situation in the future (although another change in buffer size seems unlikely)

@derrod
Copy link
Member

derrod commented Jun 7, 2024

I've been chasing too many random crashes that seem to be all caused by this so I'm just gonna merge this to preserve what remains of my sanity while working based on current master.

@derrod derrod merged commit bd36daa into obsproject:master Jun 7, 2024
15 checks passed
@RytoEX
Copy link
Member

RytoEX commented Jun 7, 2024

looks good to me 👍 the only suggestion I'd have beyond this is to move the buffer size into a constant somewhere to avoid this situation in the future (although another change in buffer size seems unlikely)

Agree with this as a welcome future change.

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.

4 participants