[gui] reset addrProxy/addrSeparateProxyTor if colon char missing #11448

1. 
   mess110 commented at 9:57 pm on October 3, 2017: contributor

   If addrProxy or addrSeparateProxyTor do not have a colon in the string somewhere in the QSettings storage, then attempting to open the options dialog will cause the entire program to crash.

   Fixes #11209

2. fanquake added the label GUI on Oct 3, 2017
3. fanquake added the label P2P on Oct 3, 2017
4. [gui] reset addrProxy/addrSeparateProxyTor if colon char missing

   If addrProxy or addrSeparateProxyTor do not have a colon in the string
   somewhere in the QSettings storage, then attempting to open the options
   dialog will cause the entire program to crash.

   ce2418fa4c
5. in src/qt/optionsmodel.cpp:129 in 71ce95dad6 outdated

   125@@ -126,6 +126,8 @@ void OptionsModel::Init(bool resetSettings)
   126         settings.setValue("fUseProxy", false);
   127     if (!settings.contains("addrProxy"))
   128         settings.setValue("addrProxy", "127.0.0.1:9050");
   129+    if (!settings.value("addrProxy").toString().contains(':'))
   

   ---

   ---

   

   achow101 commented at 10:06 pm on October 3, 2017:

   Can you just make this an OR with the if statement above?

   ---

   

   mess110 commented at 10:09 pm on October 3, 2017:

   Done
6. mess110 force-pushed on Oct 3, 2017
7. 
   meshcollider commented at 1:33 am on October 4, 2017: contributor

   Please change PR description to say ‘Fixes #11209’ so that GitHub auto-closes the issue if this is merged.

   If for some reason there was no colon, why doesn’t the default port just get appended to the existing IP rather than defaulting too?

8. 
   jonasschnelli commented at 6:04 am on October 4, 2017: contributor

   Can’t reproduce the #11209 crash on OSX. Also, there are probably other situations where a corrupted or manually edited QSettings files may lead to a crash.

   But this is better then noting.

   Concept ACK

9. 
   jonasschnelli commented at 6:04 am on October 4, 2017: contributor

   utACK ce2418fa4cef5514305cca82e74891b9d643c4c7
10. 
    mess110 commented at 7:51 am on October 4, 2017: contributor

    @MeshCollider To check this I just deleted the : from addrProxy, so the value was 127.0.0.19050. My though process was: corrupt? ok, rewrite. My goal was to avoid the crash.
11. 
    meshcollider commented at 7:59 am on October 4, 2017: contributor

    Fair enough I guess. utACK
12. 
    laanwj commented at 12:28 pm on October 4, 2017: member

    utACK,

    though I’m not sure I agree the original issue is really an issue - this can only be reached by manually tampering with the settings, which can mess up the client in a lot of ways.

13. 
    achow101 commented at 2:00 pm on October 4, 2017: member

    utACK ce2418fa4cef5514305cca82e74891b9d643c4c7
14. 
    promag commented at 10:38 pm on October 6, 2017: member

    AFAIK it doesn’t fix the issue when the setting is changed in runtime externally.

    I think the settings value should be validated when used. In this case the .split(":")[1] wouldn’t crash. This is also relevant when a setting can be upgraded/extended to a new format and/or have default values for the missing bits (port in this case).

    In this particular case I would say the fix could be the introduction/usage of a decent address parser.

    utACK ce2418f considering it’s an remote use case.

15. 
    laanwj commented at 2:07 pm on October 9, 2017: member

    AFAIK it doesn’t fix the issue when the setting is changed in runtime externally.

    The setting is only ever loaded from the qsettings at startup. Changing it at runtime externally does nothing.

16. laanwj merged this on Oct 9, 2017
17. laanwj closed this on Oct 9, 2017

    ---

18. laanwj referenced this in commit d473e6ddc5 on Oct 9, 2017
19. mess110 deleted the branch on Oct 9, 2017
20. laanwj referenced this in commit e8f10248cf on Dec 1, 2017
21. laanwj referenced this in commit efcd4cde7d on Dec 1, 2017
22. laanwj referenced this in commit f05d349887 on Dec 7, 2017
23. laanwj referenced this in commit 80f9dad0b7 on Dec 7, 2017
24. HashUnlimited referenced this in commit 86dec39120 on Mar 15, 2018
25. PastaPastaPasta referenced this in commit 88197c7414 on Jan 26, 2020
26. PastaPastaPasta referenced this in commit abb27edc72 on Feb 13, 2020
27. PastaPastaPasta referenced this in commit ea94b5dd35 on Feb 27, 2020
28. PastaPastaPasta referenced this in commit 315504c8c3 on Feb 27, 2020
29. ckti referenced this in commit 3216ba5b54 on Mar 28, 2021
30. ckti referenced this in commit 0cc2f4216b on Mar 28, 2021
31. DrahtBot locked this on Sep 8, 2021

Contributors
mess110 achow101 meshcollider jonasschnelli laanwj promag

Labels
GUI P2P