Protect waveformmark.h's operator from causing segfaults #4781
Conversation
I get a segfault here every few hours. What's happening ? Let's not call methods of waveformmarks if they are nullptr's as a dummy protection mechanism.
|
Welcome and thank you for deciding to contribute. The fix is relevant for release 2.3. Please create a separate, local bugfix branch based on I will add a code comment to this PR. |
|
The crash doesn't happen in 2.3. It does in 2.4. |
| double rightPosition = rhs->getSamplePosition(); | ||
| int rightHotcue = rhs->getHotCue(); | ||
| if (leftPosition == rightPosition) { | ||
| //A segfault in here crashes the whole program for me every few hours. Let's make sure we aren't comparing null pointers. |
There was a problem hiding this comment.
Please use VERIFY_OR_DEBUG_ASSERT(lhs) / VERIFY_OR_DEBUG_ASSERT(rhs) for each of the arguments followed by an early return. This is more readable and will also leave the remaining code unchanged.
There was a problem hiding this comment.
The debug assertions will help to reveal situations in which this bug occurs and only stop Mixxx during development, before any user notices.
|
We also need your permission before merging any code. Please sign our Contributor Agreement. |
Checking the pointers before dereferencing is still good practice and should also be added to 2.3. Just in case. |
|
For more information about the Git workflow and dev tools for code formatting or the pre-commit checks please refer to |
|
Have you enabled caching of waveform data? I have disabled it and never experienced such a crash. |
|
IMO this is also just a workaround, knowing why these actually become |
|
The segmentation fault is still occuring, seems like this quick fix is not enough: I don't know what to do with this, I'll probably just won't use waveform marks in my skin. |
|
It would be great if you could run mixxx in a debugger to capture a backtrace for when these segfaults occur. |
|
I don't think the fault lies solely in the waveform marks, the culprit must be somewhere else. If you don't use any waveform marks, mixxx will probably just crash somewhere else. |
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
@bencejuhaasz This should be fixed by #11744 , can you test this please. |
Every few hours while playing from a rekordbox usb, I got crashes because of this operator.
Checking for nullptr seems to fix the issue, I did a 5-6 hour test w/o any problem.
Not properly displaying hotcue tags seems to be a reasonable tradeoff to avoid a crash while playing live,
so I'm making a pull request. But if a simple 'false' is not the best return value, and we should check for the two marks separately, feel free to correct me.