qt: Avoid crash on startup if int specified in settings.json #24498

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:pr/setint changing 7 files +180 −1
  1. ryanofsky commented at 6:44 pm on March 7, 2022: member

    Should probably add this change to 23.x as suggested by Luke #24457 (comment). If settings like prune are added to settings.json in the future, it would be preferable for 23.x releases to respect the setting instead of crash.

    Fix GUI startup crash reported by Rspigler in #24457 that happens if settings.json contains an integer value for any of the configuration options which GUI settings can currently clash with (-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy, -proxy, -onion, -onion, -lang, and -prune).

    The fix is a one-line change in ArgsManager::GetArg. The rest of the PR just adds a regression test for the GUI and unit tests for ArgsManager::GetArg methods.

  2. test: Add tests for GetArg methods / settings.json type coercion 
    Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.

Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
    84b0973e35
  3. qt: Avoid crash on startup if int specified in settings.json 
    Fix GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 that happens if
settings.json contains an integer value for any of the configuration
options which GUI settings can currently clash with (-dbcache, -par,
-spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy,
-proxy, -onion, -onion, -lang, and -prune).

Fix is a one-line change in ArgsManager::GetArg.
    5b1aae12ca
  4. DrahtBot added the label Build system on Mar 7, 2022
  5. DrahtBot added the label GUI on Mar 7, 2022
  6. DrahtBot added the label Utils/log/libs on Mar 7, 2022
  7. DrahtBot commented at 11:52 pm on March 7, 2022: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg by luke-jr)
    • #15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  8. ryanofsky force-pushed on Mar 8, 2022
  9. ryanofsky commented at 5:13 pm on March 8, 2022: member
    Updated 81adab4ac31e6ebaf1e835168d33c7862cc904b3 -> 560431097d52b1eaacdef803b99cdd4c9d9dbd6a (pr/setint.1 -> pr/setint.2, compare) just splitting into two commits with new tests in separate commit, so this is easier to review
  10. in src/util/system.cpp:594 in 560431097d outdated 
    590@@ -591,7 +591,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
591 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
592 {
593     const util::SettingsValue value = GetSetting(strArg);
594-    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str();
595+    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.getValStr();
    luke-jr commented at 6:26 pm on March 8, 2022:
    This also returns "" for an Object or Array. Is that desirable?
    ryanofsky commented at 7:32 pm on March 8, 2022:

    This also returns "" for an Object or Array. Is that desirable?

    Good point, it does seem better not to change that behavior here, since the goal of this PR is just to prevent the GUI crashing if someone has a settings.json file with a valid int setting.

    I’ll update the PR to avoid changing array/object behavior here and add test coverage for it.

    After this, it’s probably debatable what behavior would be best. The ArgsManager::GetSetting method always lets you get the raw json value, but ArgsManager::Get(Int,Bool,Path)Arg methods are supposed to be convenient helper methods for handling default values and type coercion. If an object or array value is set and scalar value is requested, potential options seem to be:

    1. Trigger an assert or exception (current behavior)
    2. Return sentinel values like "" or 0 or false
    3. Return the default value, treat as if setting was not set
    4. Do something more fancy like return the first or last value in the array, or return the object as a string.

    After listing these, 1 does actually seem like the best option to me, but I wouldn’t be surprised if maybe there are cases where something like 3 or 4 could make more sense.

    promag commented at 9:48 pm on March 8, 2022:
    How about 5. fail to initialize with an error message?
    ryanofsky commented at 10:43 pm on March 8, 2022:

    How about 5. fail to initialize with an error message?

    Good point. That really does make the most sense and would be good to implement outside of GetArg (obviously GetArg is called many places after initialization where it is too late to stop initialization). #17580 actually does most of the work of needed to implement startup rejection of arrays and objects since it adds an ALLOW_LIST flag to all list settings and disallows GetArg even being called when ALLOW_LIST is specified. #17580 currently depends on #16545, but I could probably rebase it to be independent.

    To be sure though, all of this is off topic from the original issue and current PR.

    This PR is just one-line change to prevent the gui aborting when a valid integer setting is specified because the isNum() case was unhandled. The diff that luke was responding to accidentally changed the behavior for arrays and objects as well as numbers, but the current PR strictly limits the change to the isNum case and doesn’t affect arrays or objects.

    promag commented at 11:19 pm on March 8, 2022:
    Yes, just wanted to note that exiting with an error is better than a crash.
  11. ryanofsky force-pushed on Mar 8, 2022
  12. ryanofsky commented at 8:05 pm on March 8, 2022: member
    Updated 560431097d52b1eaacdef803b99cdd4c9d9dbd6a -> e584e1e32f4b1d20e9289c1364fff7cbb8a57c73 (pr/setint.2 -> pr/setint.3, compare) avoiding change in behavior if an array or object is specified in settings.json (still makes GUI abort with an error if a scalar value was expected). Also added test coverage for these cases.
  13. ryanofsky force-pushed on Mar 8, 2022
  14. ryanofsky commented at 8:56 pm on March 8, 2022: member
    Updated e584e1e32f4b1d20e9289c1364fff7cbb8a57c73 -> cfbcdd3001e0601a55ef4a4e9eed64e9d67e03ee (pr/setint.3 -> pr/setint.4, compare) adding more tests to deal cover more cases: zero values, nulls, empty strings, and floats Updated cfbcdd3001e0601a55ef4a4e9eed64e9d67e03ee -> 5b1aae12ca4a99c6b09349981a4902717a6a5d3e (pr/setint.4 -> pr/setint.5, compare) deduping and fixing up new test cases
  15. ryanofsky force-pushed on Mar 8, 2022
  16. in src/qt/test/optiontests.h:8 in 5b1aae12ca 
    0@@ -0,0 +1,25 @@
1+// Copyright (c) 2019 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_QT_TEST_OPTIONTESTS_H
6+#define BITCOIN_QT_TEST_OPTIONTESTS_H
7+
8+#include <qt/optionsmodel.h>
    promag commented at 11:12 pm on March 8, 2022:

    84b0973e35dae63cd1b60199b481e24d54e58c97

    nit, move to .cpp

  17. promag commented at 11:16 pm on March 8, 2022: member

    Tested ACK 84b0973e35dae63cd1b60199b481e24d54e58c97.

    Test regression fails without the fix:

    0FAIL!  : OptionTests::optionTests() Caught unhandled exception
1   Loc: [qtestcase.cpp(1939)]
2Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted, 1ms
3********* Finished testing of OptionTests *********
4libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: JSON value is not a string as expected
  18. in src/util/system.cpp:594 in 5b1aae12ca 
    590@@ -591,7 +591,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
591 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
592 {
593     const util::SettingsValue value = GetSetting(strArg);
594-    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str();
595+    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str();
    laanwj commented at 10:20 am on March 9, 2022:
    I think we’ve exceeded the threshold where the ternary operator is a helpful shorthand in this case. I’d really prefer to write this out in code (doesn’t need to be in this PR, to be clear, but this is some terrible code to review imo).
    promag commented at 10:38 am on March 9, 2022:

    Had the same feeling, this is what I did locally:

    0if (value.isNull()) return strDefault;
1if (value.isBool()) return value.isFalse() ? "0" : "1";
2if (value.isNum()) return value.getValStr();
3return value.get_str();
    MarcoFalke commented at 10:47 am on March 9, 2022:
    In some cases the ternary operator is used in place for switch-case. If this is one such case, I think it is acceptable. Though, it might be better to put each case onto a new line to clarify that.
    ryanofsky commented at 4:00 pm on March 9, 2022:

    In some cases the ternary operator is used in place for switch-case. If this is one such case, I think it is acceptable. Though, it might be better to put each case onto a new line to clarify that.

    Yes. I was about to update this but it got merged. I don’t think it is too bad, though. The line is just meant to be read as “If isNull, return default, if isFalse, return 0, if isTrue, return 1, if isNum, return getValStr, otherwise return get_str.”

    MarcoFalke commented at 5:12 pm on March 9, 2022:

    For reference my clang-format seems to understand this as well and produce:

     0diff --git a/src/util/system.cpp b/src/util/system.cpp
 1index 8e45453d31..8f37b030c1 100644
 2--- a/src/util/system.cpp
 3+++ b/src/util/system.cpp
 4@@ -591,7 +591,11 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
 5 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
 6 {
 7     const util::SettingsValue value = GetSetting(strArg);
 8-    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str();
 9+    return value.isNull()  ? strDefault :
10+           value.isFalse() ? "0" :
11+           value.isTrue()  ? "1" :
12+           value.isNum()   ? value.getValStr() :
13+                             value.get_str();
14 }
15 
16 int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const
  19. laanwj commented at 10:23 am on March 9, 2022: member
    Code review ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
  20. laanwj added this to the milestone 23.0 on Mar 9, 2022
  21. in src/qt/test/optiontests.cpp:19 in 5b1aae12ca 
    14+
15+//! Entry point for BitcoinApplication tests.
16+void OptionTests::optionTests()
17+{
18+    // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure
19+    // that setting integer prune value doesn't cause an exception to be thrown
    jonatack commented at 1:23 pm on March 9, 2022:

    84b0973e35dae63cd1b60199b481e24d54e58c97 nit if you retouch (this is correct in the preceding commit 84b0973e)

    0    // that setting an integer prune value doesn't cause an exception to be thrown
  22. jonatack commented at 1:25 pm on March 9, 2022: member

    Thanks for adding the tests!

    Code review ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e

  23. achow101 commented at 3:52 pm on March 9, 2022: member
    ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
  24. achow101 merged this on Mar 9, 2022
  25. achow101 closed this on Mar 9, 2022
  26. achow101 referenced this in commit 4607f700d0 on Mar 9, 2022
  27. achow101 referenced this in commit 7e1b968f61 on Mar 9, 2022
  28. fanquake commented at 4:07 pm on March 9, 2022: member
    Backported in #24511.
  29. sidhujag referenced this in commit b503dd4930 on Mar 9, 2022
  30. fanquake referenced this in commit 430808ab13 on Mar 10, 2022
  31. ryanofsky referenced this in commit 344537cf04 on May 20, 2022
  32. laanwj referenced this in commit 04fdd644b4 on May 25, 2022
  33. reddink referenced this in commit 9d77d50cba on May 26, 2022
  34. psgreco referenced this in commit fa72859959 on Dec 7, 2022
  35. psgreco referenced this in commit d407fb6168 on Dec 7, 2022
  36. backpacker69 referenced this in commit a9f0314e4d on Jan 18, 2023
  37. backpacker69 referenced this in commit 44df42fbb4 on Jan 18, 2023
  38. DrahtBot locked this on Mar 9, 2023


ryanofsky DrahtBot luke-jr promag laanwj MarcoFalke jonatack achow101 fanquake

Labels
GUI Build system Utils/log/libs

Milestone
23.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me