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

luke-jr commented at 8:53 PM on March 5, 2022: member

Currently, Number values can cause ~~GetArg/~~ GetBoolArg to throw an exception, which most of the code isn't expecting (eg #24457).

AFAIK it isn't possible to normally get Numbers in settings right now, but ~~that's expected to change with #15936 (which could create downgrading issues without this fixed), and in any case~~ it isn't a great idea to randomly crash because of a logically-value settings.json value anyway.

This doesn't fix Get*Arg from throwing in the case it encounters an Array or Object. It's not obvious how to handle those scenarios. However, the functions touched are refactored to explicitly clarify that those scenarios will throw.

~~NOTE: Based on 0.20 branch-point, so a clean merge to everything newer. For testing, you will probably want to merge onto 23.x or master.~~

DrahtBot added the label Utils/log/libs on Mar 5, 2022

unknown approved

unknown commented at 7:16 AM on March 6, 2022: none

utACK https://github.com/bitcoin/bitcoin/pull/24479/commits/3766c66b4f6126c30a254a9c0f61602d7a42eaaf

Agree with the concept and couldn't find any issues with the code.

DrahtBot commented at 9:08 AM on March 6, 2022: contributor

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

Conflicts

No conflicts as of last run.

luke-jr force-pushed on Mar 9, 2022

luke-jr commented at 5:03 PM on March 9, 2022: member

Rebased due to merge of partial fix in #24498

fanquake commented at 11:36 AM on March 11, 2022: member

The unit tests are failing:

test/getarg_tests.cpp(103): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(104): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(110): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(111): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(117): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(118): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(53): Leaving test case "setting_args"; testing time: 63184us

fanquake requested review from ryanofsky on Mar 11, 2022

luke-jr commented at 8:24 PM on March 11, 2022: member

Fixed tests.

luke-jr force-pushed on Mar 11, 2022

in src/util/system.cpp:681 in 3372227949 outdated

 616 | +            assert(false);
 617 | +        case UniValue::VSTR:
 618 | +        case UniValue::VNUM:
 619 | +            return InterpretBool(value.getValStr());
 620 | +    }
 621 | +    assert(false);

ryanofsky commented at 4:18 AM on March 16, 2022:

In commit "Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg" (33722279495ebf566436a274765068217b2d3543)

This is pretty verbose and a little hard to follow. Could consider stealing Marco's style from #24498 (review) and writing something like:

return value.isNull() ? fDefault :
       value.isBool() ? value.get_bool() :
       value.isNum()  ? InterpretBool(value.getValStr()) :
                        InterpretBool(value.get_str());

luke-jr commented at 2:39 PM on March 16, 2022:

Huh? I changed it to a switch because it's very readable and the repeated trinary operator is hard to follow...

ryanofsky approved

ryanofsky commented at 4:24 AM on March 16, 2022: contributor

I could see people having different opinions about this change. Personally I wouldn't object to it, but I also don't think it is an improvement overall.

The PR has changed since it was opened, but now it is pretty simple. Right now, if a user edits their settings.json file, or if the settings.json file gets corrupted some other way so it contains a numeric int or float value where a boolean value is expected, the current GetBoolArg method will throw an exception. This PR changes GetBoolArg behavior in that case to return false if the number is zero and true if the number is nonzero instead of throwing an exception.

Since this PR does not change behavior in other cases, GetBoolArg still throws exceptions if the settings value is an array or object, and GetBoolArg still has strange behavior for string values of returning true for the empty string "", true for strings which are non-zero decimal numbers, and false for all other strings.

Since this PR isn't removing every case where GetBoolArg throws exceptions, and since specifying a number where a boolean value is expected is a real indication of corruption or misconfiguration, I think throwing an exception is probably the best thing method can do right now to deal with whatever bad value is causing the problem. In longer run, if a change like #16545 goes through, settings could be typed, and errors could be triggered earlier on startup before an exception could happen here.

Anyway, this is not a very strong opinion, and I'm fine with other approaches.

Code review ACK 33722279495ebf566436a274765068217b2d3543

DrahtBot added the label Needs rebase on May 26, 2022

Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg 8a5324a666

luke-jr force-pushed on Aug 31, 2022

DrahtBot removed the label Needs rebase on Aug 31, 2022

DrahtBot added the label Needs rebase on Apr 21, 2023

DrahtBot commented at 10:38 AM on April 21, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot commented at 12:34 AM on August 29, 2023: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

achow101 commented at 4:03 PM on September 20, 2023: member

The PR didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs.

Closing due to lack of interest.

achow101 closed this on Sep 20, 2023

bitcoin locked this on Sep 19, 2024