libobs: Disallow overwriting the crash handler #12636
Conversation
gxalpha
force-pushed
the
don't-overwrite-crash-handler
branch
from
90ffb92 to
346f27d
Compare
libobs/util/base.c
Outdated
| blog(LOG_ERROR, "Tried to set a crash handler when one already exists." | ||
| "This is unsupported, and will crash in a future version of OBS."); |
There was a problem hiding this comment.
Do we want to promise crashing in future versions? Making this a no-op seems fine.
There was a problem hiding this comment.
Just adding I also agree that we don't need to cause a crash, we can log the error and leave it a no-op.
There was a problem hiding this comment.
Calling this a second time is now bad behavior, and we should push devs to fix it. So I‘d argue we should make it crash eventually, that would effectively make this like a deprecated method. Otherwise people might not even notice, or care enough to remove the function call (which they should).
(See e.g. bmalloc(0) as a recent-ish example, only the crashes made people really care)
There was a problem hiding this comment.
I don't disagree on the bmalloc(0) assessment, but I'm not sure we're ready to commit to crashing this in a future release. Maybe we'll change our opinion later.
There was a problem hiding this comment.
Removed the commitment to crashing.
There was a problem hiding this comment.
I noticed that when no crash log is produced, the crash handler is not entered in the first place.
Also, this will not prevent anyone from setting an exception handler using Win32 API, so the validity of the claim that this will prevent masking crash reports is a bit fuzzy to say the least.
What would be great instead, is tools which would help debugging using OBS-produced crash reports and/or a memory minidump produced in proximity to the crash log.
There was a problem hiding this comment.
Another great option would be to have a crash handling callback after OBS has done it's thing.
There was a problem hiding this comment.
As noted in the PR description, the goal of this PR is to prevent OBS' crash handler from being overwritten. Nowhere do I claim that this is supposed to and/or will prevent making crash reports, if using the Win32 API works and doesn't overwrite OBS' handler, no one (except maybe data privacy laws) is stopping you from using that.
There was a problem hiding this comment.
Would you be open to removing exit() call from the OBS native crash handler, so whatever code runs a crash handler afterwards would be able to pick up it's output?
There was a problem hiding this comment.
I will defer to others as to what is okay to do there. However, I consider that to be out-of-scope of this PR.
gxalpha
force-pushed
the
don't-overwrite-crash-handler
branch
2 times, most recently
from
13b8cb7 to
c76a72f
Compare
Setting a new crash handler overwrites the existing one. This is not desirable.
gxalpha
force-pushed
the
don't-overwrite-crash-handler
branch
from
c76a72f to
bfc7f67
Compare
|
It's a good thing I looked at this PR again with a functioning brain, now it actually does what it advertises instead of simply rejecting any call. |
Setting a new crash handler overwrites the existing one. This is not desirable.
Description
Makes it illegal (and impossible*) to overwrite the existing crash handler.
If a crash handler is set already, calling this method will now be no-op.
*Nothing is impossible if you have access to the application's memory, but... :P
Motivation and Context
We set a crash handler in OBS Studio, and we'd like to keep it.
Sometimes OBS doesn't generate a crash log, and I suspect this could be the reason why.
How Has This Been Tested?
On Windows: Hasn't, because no Windows.
On macOS: Tested that the first call to
base_set_crash_handlerworks, and any following call gets rejected.Types of changes
Checklist: