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

Prevent segmentation faults caused by unfavorable ordering of static destructors #83

Merged
merged 1 commit into from
Oct 11, 2016
Merged

Prevent segmentation faults caused by unfavorable ordering of static destructors #83

merged 1 commit into from
Oct 11, 2016

Conversation

kempniu
Copy link
Contributor

@kempniu kempniu commented Oct 11, 2016

Invoking the QSettings destructor from a destructor of a static variable (static QNapiConfig cfg) is a bad idea. In case the configuration file has not been changed since it was last sync()'ed, QConfFileSettingsPrivate::syncConfFile() will create a QFileInfo instance and then call its lastModified() method, which (a few calls down the chain) depends on the default constructor of QDateTime() to succeed. The problem with that constructor is that (at least until Qt 5.7 inclusive) it calls defaultDateTimePrivate(), which is defined using Q_GLOBAL_STATIC_WITH_ARGS (so it is basically another static entity whose destructor is called either before or after the QNapiConfig destructor). If one is lucky, the cfg singleton will be destroyed before defaultDateTimePrivate and things will work just fine, but this ordering is not guaranteed. For example, on Arch Linux with Qt 5.7, QNapi will segfault if you invoke it using qnapi -o and then press the Save button in the dialog box.

As sync() is explicitly invoked in QNapiConfig::save(), there is no practical need to call delete for the QSettings instance as the only thing its destructor does is syncing the changes (i.e. it is equivalent to calling sync() explicitly). As QNapiConfig is a singleton, no memory leak is possible here either as the OS will free any resources allocated to the program upon its exit anyway.

However, this issue might be a sign that perhaps the singleton pattern needs to be reimplemented in a safer manner.

@krzemin
Copy link
Member

krzemin commented Oct 11, 2016

Thanks for elaborating. To be honest I would get rid of all singletons and shared mutable state, but the code is mostly legacy and it's a lot of work.

@krzemin krzemin merged commit aafbf76 into QNapi:master Oct 11, 2016
@kempniu kempniu deleted the fix-segfault-static-dtors branch October 13, 2016 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants