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

GlobalShortcutMac: fix segmentation fault in setEnabled() #3603

Conversation

@davidebeatrici
Copy link
Member

commented Feb 20, 2019

Fixes #3471.

Since macOS Mojave, passing NULL to CGEventTapEnable() causes a segmentation fault.

This pull request also:

  • Initializes pointers to NULL in the constructor.
  • Adds a check in the destructor so that NULL is not passed to CFRunLoopStop().
  • Adds a check to enabled(), so that NULL is not passed to CGEventTapIsEnabled().

@davidebeatrici davidebeatrici requested review from mkrautz, Kissaki and hacst Feb 20, 2019

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-cgeventtapenable-segfault-fix branch from c328246 to 3a441e1 Feb 20, 2019

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-cgeventtapenable-segfault-fix branch 2 times, most recently from 85c022b to 832be5b Mar 21, 2019

#include "Global.h"

// We can't mark it as translation in GlobalShortcut_macx.mm because it's not processed by 'lupdate'.
static const char *osPrivacySettingsError = QT_TRANSLATE_NOOP("GlobalShortcutMac", "Unable to initialize the global shortcuts handler. Please check that your operating system's privacy settings allow Mumble to use the accessibility functions.");

This comment has been minimized.

Copy link
@hacst

hacst Mar 28, 2019

Member

According to the lupdate man page there is a parameter for specifying the extensions it checks:

       -extensions <ext>[,<ext>...]
              Process  files  with  the  given  extensions only.  The extension list must be separated with commas, not with whitespace.  Default:
              'ui,c,c++,cc,cpp,cxx,ch,h,h++,hh,hpp,hxx'.

Instead of putting it in the header we should add .mm to that list in our updatetranslations.sh. That should hopefully do the trick and prevent mistakes in the future.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Apr 4, 2019

Author Member

Unfortunately it doesn't work, lupdate doesn't parse Objective-C++: https://bugreports.qt.io/browse/QTBUG-44808

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Apr 4, 2019

Author Member

I propose to create a function in GlobalShortcut which shows the translated message.

What do you think about that?

qWarning("GlobalShortcutMac: Unable to create EventTap. Global Shortcuts will not be available.");
g.mw->msgBox(QLatin1String(osPrivacySettingsError));

This comment has been minimized.

Copy link
@hacst

hacst Mar 28, 2019

Member

Does this work? I'm kinda doubtful g.mw exists at that point. From a quick check it looks like global shortcuts are created in the main window construction phase and before construction concludes g.mw is not set.

If that is the case maybe check QMessageBox for ways to display a modal without needing a parent. This would also allow you to get rid of the heavy includes.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 28, 2019

Author Member

g.mw->msgBox() doesn't actually show a message box, it prints the message in the chat box:

void MainWindow::msgBox(QString msg) {
MessageBoxEvent *mbe=new MessageBoxEvent(msg);
QApplication::postEvent(this, mbe);
}

This comment has been minimized.

Copy link
@hacst

hacst Mar 29, 2019

Member

I see. Maybe we should rename it ;) Doesn't resolve the issue though.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 29, 2019

Author Member

I agree, I initially though msgBox() was related to QMessageBox too.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici May 9, 2019

Author Member

I tested the pull request.

Calling g.mw->msgBox() doesn't cause a crash, however the message is not shown because the chat box is not created yet:

QCoreApplication::postEvent: Unexpected null receiver
@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

As fixing the crash is mandatory for 1.3, I'm going to remove the log part, we'll look into it in future.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-cgeventtapenable-segfault-fix branch from 832be5b to 39006b1 May 24, 2019

@davidebeatrici davidebeatrici changed the title GlobalShortcutMac: fix segmentation fault and print log message GlobalShortcutMac: fix segmentation fault in setEnabled() May 24, 2019

GlobalShortcutMac: fix segmentation fault in setEnabled()
Since macOS Mojave, passing NULL to CGEventTapEnable() causes a segmentation fault.

This commit also:

- Initializes pointers to NULL in the constructor.
- Adds a check in the destructor so that NULL is not passed to CFRunLoopStop().
- Adds a check to enabled(), so that NULL is not passed to CGEventTapIsEnabled().

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-cgeventtapenable-segfault-fix branch from 39006b1 to d7353b7 May 24, 2019

@davidebeatrici davidebeatrici merged commit 4e83913 into mumble-voip:master May 25, 2019

4 of 6 checks passed

CI Build #20190524.3 failed
Details
CI (macOS) macOS failed
Details
CI (Windows MSVC_2015) Windows MSVC_2015 succeeded
Details
CI (Windows MSVC_2015_NO_PCH) Windows MSVC_2015_NO_PCH succeeded
Details
Docker Build Task Summary
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.