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

ryanofsky · 2022-03-07T18:44:23Z

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.

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 bitcoin#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).

Fix GUI startup crash reported by Rspigler in bitcoin#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.

DrahtBot · 2022-03-07T23:52:40Z

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.

ryanofsky · 2022-03-08T17:13:03Z

Updated 81adab4 -> 5604310 (pr/setint.1 -> pr/setint.2, compare) just splitting into two commits with new tests in separate commit, so this is easier to review

src/util/system.cpp

ryanofsky · 2022-03-08T20:05:22Z

Updated 5604310 -> e584e1e (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.

ryanofsky · 2022-03-08T20:56:53Z

Updated e584e1e -> cfbcdd3 (pr/setint.3 -> pr/setint.4, compare) adding more tests to deal cover more cases: zero values, nulls, empty strings, and floats
Updated cfbcdd3 -> 5b1aae1 (pr/setint.4 -> pr/setint.5, compare) deduping and fixing up new test cases

promag

Tested ACK 84b0973.

Test regression fails without the fix:

FAIL!  : OptionTests::optionTests() Caught unhandled exception
   Loc: [qtestcase.cpp(1939)]
Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted, 1ms
********* Finished testing of OptionTests *********
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: JSON value is not a string as expected

promag · 2022-03-08T23:12:52Z

src/qt/test/optiontests.h

+#ifndef BITCOIN_QT_TEST_OPTIONTESTS_H
+#define BITCOIN_QT_TEST_OPTIONTESTS_H
+
+#include <qt/optionsmodel.h>


84b0973

nit, move to .cpp

laanwj · 2022-03-09T10:20:48Z

src/util/system.cpp

@@ -591,7 +591,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
 {
    const util::SettingsValue value = GetSetting(strArg);
-    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str();
+    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str();


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).

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

if (value.isNull()) return strDefault; if (value.isBool()) return value.isFalse() ? "0" : "1"; if (value.isNum()) return value.getValStr(); return value.get_str();

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.

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."

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

diff --git a/src/util/system.cpp b/src/util/system.cpp index 8e45453d31..8f37b030c1 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -591,7 +591,11 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const { const util::SettingsValue value = GetSetting(strArg); - return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str(); + return value.isNull() ? strDefault : + value.isFalse() ? "0" : + value.isTrue() ? "1" : + value.isNum() ? value.getValStr() : + value.get_str(); } int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const

laanwj · 2022-03-09T10:23:28Z

Code review ACK 5b1aae1

jonatack

Thanks for adding the tests!

Code review ACK 5b1aae1

jonatack · 2022-03-09T13:23:29Z

src/qt/test/optiontests.cpp

+void OptionTests::optionTests()
+{
+    // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure
+    // that setting integer prune value doesn't cause an exception to be thrown


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

Suggested change

// that setting integer prune value doesn't cause an exception to be thrown

// that setting an integer prune value doesn't cause an exception to be thrown

achow101 · 2022-03-09T15:52:17Z

ACK 5b1aae1

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 bitcoin#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). Github-Pull: bitcoin#24498 Rebased-From: 84b0973

Fix GUI startup crash reported by Rspigler in bitcoin#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. Github-Pull: bitcoin#24498 Rebased-From: 5b1aae1

fanquake · 2022-03-09T16:07:28Z

Backported in #24511.

…ettings.json 5b1aae1 qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky) 84b0973 test: Add tests for GetArg methods / settings.json type coercion (Ryan Ofsky) Pull request description: Should probably add this change to 23.x as suggested by Luke bitcoin#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 bitcoin#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. ACKs for top commit: laanwj: Code review ACK 5b1aae1 achow101: ACK 5b1aae1 jonatack: Code review ACK 5b1aae1 Tree-SHA512: 958991b4bead9b82a3879fdca0f8d6405e2a212b7c46cf356f078843a4f156e27fd75fc46e2013aa5159582ead06d343c1ed248d678b3e5bbd312f247e37894c

…ettings.json 7e1b968 qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky) 4607f70 test: Add tests for GetArg methods / settings.json type coercion (Ryan Ofsky) Pull request description: Backport of #24498 ACKs for top commit: jonatack: ACK 7e1b968 Tree-SHA512: efe6ec4361858e50fd524db64141ad3622ecef321b99567da9650575558a9a9bdec0fc43113967cae2f23a1375132eed2d6ebf64d4aa91ac1c5f2f591a26ba74

Fix GUI startup crash reported by Rspigler in bitcoin#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. Github-Pull: bitcoin#24498 Rebased-From: 5b1aae1

…ettings.json 344537c qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky) Pull request description: Backport of #24498 to 22.x branch This was already backported to the 23.x branch in #24511, but vasild discovered the issue can affect 22.x as well #15936 (comment): > While testing this, 22.x crashes if `settings.json` contains `"prune": 1234` due to #24498 which was fixed in 23.0. So, if this PR is included in 24.x and a user upgrades to 24.x and then downgrades to 22.x his 22.x would crash at startup. It's not very important to backport this to 22.x because I can work around the issue other ways, but I do see 22.x is listed as an active branch https://github.com/bitcoin/bitcoin/branches, so if there is another 22.x release, it would be nice to have this fix and not have a wonky uncaught univalue exception waiting to be hit ACKs for top commit: laanwj: ACK 344537c Tree-SHA512: 479f7fb4b4b73ec85f28e97af9b21d54245b8ab4d246e50134b37f1726fe3c57cf388fe4973f8ccf1b9efa7663166d1fbeb631758b39d4490b5eab82d0c83d77

Fix GUI startup crash reported by Rspigler in bitcoin/bitcoin#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. Github-Pull: bitcoin/bitcoin#24498 Rebased-From: 5b1aae12ca4a99c6b09349981a4902717a6a5d3e

Fix GUI startup crash reported by Rspigler in bitcoin/bitcoin#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. Github-Pull: bitcoin/bitcoin#24498 Rebased-From: 5b1aae1 (cherry picked from commit 344537c)

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 bitcoin/bitcoin#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). Github-Pull: bitcoin/bitcoin#24498 Rebased-From: 84b0973e35dae63cd1b60199b481e24d54e58c97

Fix GUI startup crash reported by Rspigler in bitcoin/bitcoin#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. Github-Pull: bitcoin/bitcoin#24498 Rebased-From: 5b1aae12ca4a99c6b09349981a4902717a6a5d3e

ryanofsky added 2 commits March 7, 2022 13:29

ryanofsky mentioned this pull request Mar 7, 2022

Application Aborted #24457

Closed

DrahtBot added Build system GUI Utils/log/libs labels Mar 7, 2022

DrahtBot mentioned this pull request Mar 8, 2022

Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg #24479

Closed

ryanofsky force-pushed the pr/setint branch from 81adab4 to 5604310 Compare March 8, 2022 17:12

luke-jr reviewed Mar 8, 2022

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/setint branch from 5604310 to e584e1e Compare March 8, 2022 20:03

ryanofsky force-pushed the pr/setint branch from e584e1e to cfbcdd3 Compare March 8, 2022 20:55

ryanofsky force-pushed the pr/setint branch from cfbcdd3 to 5b1aae1 Compare March 8, 2022 21:13

promag reviewed Mar 8, 2022

View reviewed changes

laanwj reviewed Mar 9, 2022

View reviewed changes

laanwj added this to the 23.0 milestone Mar 9, 2022

jonatack reviewed Mar 9, 2022

View reviewed changes

achow101 merged commit 47bbd3f into bitcoin:master Mar 9, 2022

achow101 mentioned this pull request Mar 9, 2022

[23.x] qt: Avoid crash on startup if int specified in settings.json #24511

Merged

DrahtBot mentioned this pull request Mar 11, 2022

interfaces: Expose settings.json methods to GUI #15936

Merged

ryanofsky mentioned this pull request May 20, 2022

[22.x] qt: Avoid crash on startup if int specified in settings.json #25180

Merged

reddink mentioned this pull request May 26, 2022

[4.22.x] Backports for 4.22.x reddcoin-project/reddcoin#20

Merged

bitcoin locked and limited conversation to collaborators Mar 9, 2023

	// that setting integer prune value doesn't cause an exception to be thrown
	// that setting an integer prune value doesn't cause an exception to be thrown

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

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

Uh oh!

Conversation

ryanofsky commented Mar 7, 2022 • edited by achow101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 7, 2022

Conflicts

Uh oh!

ryanofsky commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 9, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 9, 2022

Uh oh!

fanquake commented Mar 9, 2022

Uh oh!

Uh oh!

ryanofsky commented Mar 7, 2022 •

edited by achow101

Loading

ryanofsky commented Mar 8, 2022 •

edited

Loading

ryanofsky commented Mar 8, 2022 •

edited

Loading

ryanofsky commented Mar 8, 2022 •

edited

Loading

laanwj Mar 9, 2022 •

edited

Loading