From 4fb01dd766c824529dad4bd1ca64a981e137d476 Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Sat, 25 Nov 2017 09:22:19 +0100 Subject: Fix ownership issues in preferences code Each preferences object owns its string members. In three cases, pointers were copied instead of strings, leading to (in the best case) dangling pointers if the user edited values: 1) In the GET_TXT macro in core/prefs-macros.h 2) In the PreferencesDialog::defaultsRequested() method 3) In main() of the mobile version This patch fixes these issues, by using copy_string() or copy_prefs() as appropriate. The only reason that the old code didn't crash regularly is that the default_prefs object was only used at startup and defaultsRequested() is (at the moment?) dead code. This patch also aligns the backslashes in core/pref.h and fixes a typo. The declaration of copy_prefs() is moved to the core/prefs.h header. Signed-off-by: Berthold Stoeger --- core/pref.h | 1 + core/prefs-macros.h | 10 +++++----- core/subsurfacestartup.h | 1 - desktop-widgets/preferences/preferencesdialog.cpp | 4 ++-- subsurface-mobile-main.cpp | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/core/pref.h b/core/pref.h index 198d4b9fe..d25115c8e 100644 --- a/core/pref.h +++ b/core/pref.h @@ -189,6 +189,7 @@ extern const char *system_default_directory(void); extern const char *system_default_filename(); extern bool subsurface_ignore_font(const char *font); extern void subsurface_OS_pref_setup(); +extern void copy_prefs(struct preferences *src, struct preferences *dest); #ifdef __cplusplus } diff --git a/core/prefs-macros.h b/core/prefs-macros.h index 6f1b665fd..15a9a9867 100644 --- a/core/prefs-macros.h +++ b/core/prefs-macros.h @@ -18,10 +18,10 @@ else \ prefs.units.field = default_prefs.units.field -#define GET_UNIT_BOOL(name, field) \ +#define GET_UNIT_BOOL(name, field) \ v = s.value(QString(name)); \ if (v.isValid()) \ - prefs.units.field = v.toBool(); \ + prefs.units.field = v.toBool(); \ else \ prefs.units.field = default_prefs.units.field @@ -53,10 +53,10 @@ else \ prefs.field = default_prefs.field -#define GET_INT_DEF(name, field, defval) \ +#define GET_INT_DEF(name, field, defval) \ v = s.value(QString(name)); \ if (v.isValid()) \ - prefs.field = v.toInt(); \ + prefs.field = v.toInt(); \ else \ prefs.field = defval @@ -65,7 +65,7 @@ if (v.isValid()) \ prefs.field = strdup(v.toString().toUtf8().constData()); \ else \ - prefs.field = default_prefs.field + prefs.field = copy_string(default_prefs.field) #define SAVE_OR_REMOVE_SPECIAL(_setting, _default, _compare, _value) \ if (_compare != _default) \ diff --git a/core/subsurfacestartup.h b/core/subsurfacestartup.h index b26c99025..40de09723 100644 --- a/core/subsurfacestartup.h +++ b/core/subsurfacestartup.h @@ -17,7 +17,6 @@ extern bool imported; void setup_system_prefs(void); void parse_argument(const char *arg); void free_prefs(void); -void copy_prefs(struct preferences *src, struct preferences *dest); void print_files(void); void print_version(void); diff --git a/desktop-widgets/preferences/preferencesdialog.cpp b/desktop-widgets/preferences/preferencesdialog.cpp index 6f5f4b9fa..50a0049ac 100644 --- a/desktop-widgets/preferences/preferencesdialog.cpp +++ b/desktop-widgets/preferences/preferencesdialog.cpp @@ -109,7 +109,7 @@ void PreferencesDialog::refreshPages() curr->setParent(0); } - // Readd things. + // Read things Q_FOREACH(AbstractPreferencesWidget *page, pages) { QListWidgetItem *item = new QListWidgetItem(page->icon(), page->name()); pagesList->addItem(item); @@ -139,7 +139,7 @@ void PreferencesDialog::cancelRequested() void PreferencesDialog::defaultsRequested() { - prefs = default_prefs; + copy_prefs(&default_prefs, &prefs); Q_FOREACH(AbstractPreferencesWidget *page, pages) { page->refreshSettings(); } diff --git a/subsurface-mobile-main.cpp b/subsurface-mobile-main.cpp index 0ee2890c6..63add7c50 100644 --- a/subsurface-mobile-main.cpp +++ b/subsurface-mobile-main.cpp @@ -41,7 +41,7 @@ int main(int argc, char **argv) setup_system_prefs(); if (uiLanguage(0).contains("-US")) default_prefs.units = IMPERIAL_units; - prefs = default_prefs; + copy_prefs(&default_prefs, &prefs); fill_profile_color(); fill_computer_list(); -- cgit v1.2.3-70-g09d2