Validate commandline arg values, and unify -help documentation #225

Merged
merged 4 commits into from Feb 18, 2017

Conversation

Projects
None yet
3 participants
@jamoes

jamoes commented Feb 15, 2017

This resolves #172

Re-works the AllowedArgs class to be instantiable and add methods for building and verifying the set of allowed arguments, values, and associated help message. Creates a child class of AllowedArgs for each executable, and for the config file.

In addition, this change moves several constants from .cpp files into .h files, so they are accessible from the AllowedArgs class. It also adds test cases for invalid argument names and values.


Command-line values are verified by grouping all existing arguments into one of six possible value types:

  • Optional boolean
    Argument values that are accessed using GetBoolArg are verified using the optionalBool function. The allowed values are "0", "1", or "".
  • Required string
    Argument values that are accessed using std::string GetArg(), or that are accessed through mapArgs directly are predominantly verified using the requiredStr function. This allows any value, but must not be empty.
  • Optional string
    This is an uncommon type, but some arguments accept strings while also having valid behavior with no value. These arguments are verified using the optionalStr function, and accept any value (including empty strings). (Currently only -connect, -debug, and -rootcertificates use this type).
  • Required int
    Argument values that are accessed using int64_t GetArg() are predominantly verified using the requiredInt function. This allows any decimal character (0-9), but must not be empty. It also allows a negative sign ('-') as the first character. This is used for int argument values that are not typically valid with a value of 0 (or should be explicit if they want to specify a 0 value).
  • Optional int
    Argument values that are accessed using int64_t GetArg() are sometimes verified using the optionalInt function. This allows the same values as requiredInt, but can also be empty. If the value is empty, it will be converted to 0 by GetArg, so this is used for int argument values that are valid with a value of 0 (-maxconnections and -limitfreerelay). It is also used for bool arguments that also accept an int (-upgradewallet and -zapwallettxes).
  • Required amount
    Argument values that are parsed with ParseMoney are verified using the requiredAmount function. This allows any decimal character (0-9) or a '.' character, but must not be empty.

Some arguments could have more strict checks (e.g., -checklevel only accepting values 0-4). However, the goal of this change is just to have high-level argument types. Specific handling of the values (e.g., confining them to a min/max range) should be handled elsewhere (and all the code already does this, since prior to this change, all values were accepted by the command-line parser).

Note that I considered using regular expressions to validate the argument values, however the std::regex library isn't supported in gcc versions prior to 4.9. Also, the windows executables did not work when using regular expressions. So, although creating custom helper functions for each type is more verbose than simply using a regex, this approach is more portable.


All help message documentation (the text that is output up if you run an executable with the -help flag) is moved into the AllowedArgs class. This ensures that all allowed arguments and their documentation are defined in a single location in the codebase.


The design of the AllowedArgs class is inspired by the Boost.program_options library, which allows for defining the allowed arguments for a program as well as their types and their associated help documentation. However, I was not able to simply use this library directly, because of several requirements that the bitcoin executables have that are incompatible with Boost.program_options:

Most notably, Boost.program_options does not have support for the -help-debug option in bitcoin executables, which allows for a more detailed help message to be shown. Additionally, Boost.program_options expects double-dash arguments (e.g. --options), and it generates its help documentation with double-dashes. Also, Boost.program_options allows spaces to separate argument names with values, whereas bitcoin executables require an '=' character (e.g. --option1 value1 --option2 value2 vs -option1=value1 -option2=value2). While these incompatibilities could potentially be worked around, I believe it was easier and cleaner to simply implement the AllowedArgs class, which tailors its behavior for the bitcoin executables.


On a pure line-count, this change is relatively large. However, the majority of the lines changed are simply moving the help documentation from wherever it was located in the codebase into the AllowedArgs class. In order to ease reviewing, this pull request consists of two git commits (each of which are compilable and have all tests passing):

The first commit modifies the AllowedArgs class, and defines arguments for the bitcoin-tx executable. You can look at this commit alone to understand the AllowedArg class and how it is used.

The second commit adds argument definitions for all the remaining executables (bitcoind, bitcoin-qt and bitcoin-cli). In addition, it moves several constants into .h files so that they can be accessed from the AllowedArgs class.


Note that the AllowedArgs class does not always have access to instances of the CChainParams class (since they are in different compilation units, which aren't always linked together). For this reason, I had to make new constants for DEFAULT_MAINNET_PORT and DEFAULT_TESTNET_PORT.

In addition, the help documentation for -checkblockindex and -acceptnonstdtxn relies on values from CChainParams instances. Since these arguments are -help-debug arguments, I simply hard-coded their default values into the help documentation rather than creating new esoteric constants.


In order to verify that none of the help documentation has been inadvertently modified by this change, I have diffed the output of all the executables' help output. The following command works in bash for comparing the help output of two executables (assuming the .. dir contains the executable prior to this change, and the ./src dir contains this change's executable):

diff <(../bitcoind -?) <(./src/bitcoind -?)

I ran that command for all 4 executables, with and without the -help-debug flag (except no -help-debug flag for bitcoin-tx and bitcoin-cli, which did not support that flag prior to this change).

The help documentation for all executables is the same, with the exception of a few intentional changes:

  • Creating a new 'Help options' group, and moving -?, -version, and -help-debug into that group.
  • Creating a new 'Configuration location options' group, and moving -conf and -datadir into that group.
  • Modifying chain selection options for bitcoin-tx and bitcoin-cli, so that -regtest only shows on -help-debug.
  • Renaming the 'Options' group to 'General options'.
  • Showing all valid argument names on -help-debug (e.g., showing -help and -h as alternatives to -?).
  • Adding example value (e.g. =<n>) for -bytespersigop, -datacarriersize, and -uiplatform.
  • Removing help documentation for -nodebug, since the -no* syntax is generically handled for all arguments.

A note on backward compatibility: this change will cause the executables to print an error message and immediately quit if there are any invalid command-line arguments or argument values. This has the potential to break things for users when they upgrade if they launch with invalid command-line options or if they have invalid options in their config file.

For example, a user might launch with -listen=false, which currently works as expected (because atoi("false") converts the value to 0). However, upon upgrading, their node will fail to launch. This is the intended behavior, so that users are alerted to invalid command-line flags as early as possible. E.g. prior to this change, if a user tried the inverse: -listen=true, the listen flag would actually be set to false. This change will reduce the chance of that type of unexpected behavior from occurring.

jamoes added some commits Feb 14, 2017

Add value checking and help message to AllowedArgs
Re-works the AllowedArgs class to be instantiable and add methods for
building and verifying the set of allowed arguments, values, and
associated help message. Child classes will be used for each executable
type.

Only includes argument definitions for bitcoin-tx. Arguments for other
executables will be in the next commit.
Add AllowedArgs argument definitions for bitcoin(d|-qt|-cli)
Converts all help message documentation to AllowedArgs classes. Moves
several constants from .cpp files into .h files, so they are
accessible from the AllowedArgs class. Adds test cases for invalid
argument names and values.
@zander

This comment has been minimized.

Show comment
Hide comment
@zander

zander Feb 15, 2017

The Teamcity Continues integration system found some errors.

The basic linux-debian build went fine, the unit tests and rpc tests also finished Ok for that build.
The failed ones are the gitian based ones that do static compilation against a specific set of libraries and versions.

Linux 32-bit and 64-bit compiles gave the same error;

allowed_args.cpp: In function 'void AllowedArgs::addZmqOptions(AllowedArgs::AllowedArgs&)':
allowed_args.cpp:328:9: error: expected unqualified-id before '.' token
ICECC[562] 09:57:52: Compiled on 192.168.3.120
make[2]: *** [libbitcoin_util_a-allowed_args.o] Error 1
make[2]: *** Waiting for unfinished jobs....

MacOs compile (uses clang)

gcc -I. -g -O2 -Wall -Wextra -Wno-unused-function -c src/gen_context.c -o gen_context.o
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/clang++ -target x86_64-apple-darwin11 -mmacosx-version-min=10.7 --sysroot /home/builder/TeamCity/work/6ff1207d8200ef46/depends/SDKs/MacOSX10.9.sdk -mlinker-version=241.9 -stdlib=libc++ -std=c++11 -I. -I./include -std=c++0x  -DOS_MACOSX -DLEVELDB_PLATFORM_POSIX -DLEVELDB_ATOMIC_PRESENT -Wstack-protector -fstack-protector-all -fPIE -pipe -O2  -fvisibility=hidden -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -D__STDC_LIMIT_MACROS -c helpers/memenv/memenv.cc -o helpers/memenv/memenv.o
  CXX      bitcoin_cli-bitcoin-cli.o
In file included from allowed_args.cpp:5:
In file included from ./allowed_args.h:8:
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:589:31: error: static_cast from 'pointer' (aka 'std::__1::__list_node_base<AllowedArgs::HelpComponent, void *> *') to '__node_const_pointer' (aka 'std::__1::__list_node<AllowedArgs::HelpComponent, void *> *'), which are not related by inheritance, is not allowed
        return const_iterator(static_cast<__node_const_pointer>(
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:884:59: note: in instantiation of member function 'std::__1::__list_imp<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::end' requested here
    const_iterator end()    const _NOEXCEPT {return base::end();}
                                                          ^
allowed_args.cpp:107:38: note: in instantiation of member function 'std::__1::list<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::end' requested here
    for (HelpComponent helpComponent : m_helpList)
                                     ^
  CXX      libbitcoin_cli_a-rpcclient.o
3 errors generated.
make[2]: *** [libbitcoin_util_a-allowed_args.o] Error 1
make[2]: *** Waiting for unfinished jobs....
Makefile:4140: recipe for target 'libbitcoin_util_a-allowed_args.o' failed
rm -f libmemenv.a
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/x86_64-apple-darwin11-ar -rs libmemenv.a helpers/memenv/memenv.o
x86_64-apple-darwin11-ar: creating archive libmemenv.a
make[3]: Leaving directory '/home/builder/TeamCity/work/6ff1207d8200ef46/src/leveldb'
In file included from util.cpp:10:
In file included from ./util.h:17:
In file included from ./allowed_args.h:8:
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:684:30: error: static_cast from 'pointer' (aka 'std::__1::__list_node_base<AllowedArgs::HelpComponent, void *> *') to '__node_pointer' (aka 'std::__1::__list_node<AllowedArgs::HelpComponent, void *> *'), which are not related by inheritance, is not allowed
        __node_pointer __l = static_cast<__node_pointer>(
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:670:5: note: in instantiation of member function 'std::__1::__list_imp<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::clear' requested here
    clear();
    ^
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:780:29: note: in instantiation of member function 'std::__1::__list_imp<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::~__list_imp' requested here
class _LIBCPP_TYPE_VIS_ONLY list
                            ^
2 errors generated.
Makefile:4308: recipe for target 'libbitcoin_util_a-util.o' failed
make[2]: *** [libbitcoin_util_a-util.o] Error 1
gcc gen_context.o -o gen_context

Compile on ARM;

allowed_args.cpp: In function 'void AllowedArgs::addZmqOptions(AllowedArgs::AllowedArgs&)':
allowed_args.cpp:328:9: error: expected unqualified-id before '.' token
         .addHeader(_("ZeroMQ notification options:"))

zander commented Feb 15, 2017

The Teamcity Continues integration system found some errors.

The basic linux-debian build went fine, the unit tests and rpc tests also finished Ok for that build.
The failed ones are the gitian based ones that do static compilation against a specific set of libraries and versions.

Linux 32-bit and 64-bit compiles gave the same error;

allowed_args.cpp: In function 'void AllowedArgs::addZmqOptions(AllowedArgs::AllowedArgs&)':
allowed_args.cpp:328:9: error: expected unqualified-id before '.' token
ICECC[562] 09:57:52: Compiled on 192.168.3.120
make[2]: *** [libbitcoin_util_a-allowed_args.o] Error 1
make[2]: *** Waiting for unfinished jobs....

MacOs compile (uses clang)

gcc -I. -g -O2 -Wall -Wextra -Wno-unused-function -c src/gen_context.c -o gen_context.o
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/clang++ -target x86_64-apple-darwin11 -mmacosx-version-min=10.7 --sysroot /home/builder/TeamCity/work/6ff1207d8200ef46/depends/SDKs/MacOSX10.9.sdk -mlinker-version=241.9 -stdlib=libc++ -std=c++11 -I. -I./include -std=c++0x  -DOS_MACOSX -DLEVELDB_PLATFORM_POSIX -DLEVELDB_ATOMIC_PRESENT -Wstack-protector -fstack-protector-all -fPIE -pipe -O2  -fvisibility=hidden -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -D__STDC_LIMIT_MACROS -c helpers/memenv/memenv.cc -o helpers/memenv/memenv.o
  CXX      bitcoin_cli-bitcoin-cli.o
In file included from allowed_args.cpp:5:
In file included from ./allowed_args.h:8:
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:589:31: error: static_cast from 'pointer' (aka 'std::__1::__list_node_base<AllowedArgs::HelpComponent, void *> *') to '__node_const_pointer' (aka 'std::__1::__list_node<AllowedArgs::HelpComponent, void *> *'), which are not related by inheritance, is not allowed
        return const_iterator(static_cast<__node_const_pointer>(
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:884:59: note: in instantiation of member function 'std::__1::__list_imp<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::end' requested here
    const_iterator end()    const _NOEXCEPT {return base::end();}
                                                          ^
allowed_args.cpp:107:38: note: in instantiation of member function 'std::__1::list<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::end' requested here
    for (HelpComponent helpComponent : m_helpList)
                                     ^
  CXX      libbitcoin_cli_a-rpcclient.o
3 errors generated.
make[2]: *** [libbitcoin_util_a-allowed_args.o] Error 1
make[2]: *** Waiting for unfinished jobs....
Makefile:4140: recipe for target 'libbitcoin_util_a-allowed_args.o' failed
rm -f libmemenv.a
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/x86_64-apple-darwin11-ar -rs libmemenv.a helpers/memenv/memenv.o
x86_64-apple-darwin11-ar: creating archive libmemenv.a
make[3]: Leaving directory '/home/builder/TeamCity/work/6ff1207d8200ef46/src/leveldb'
In file included from util.cpp:10:
In file included from ./util.h:17:
In file included from ./allowed_args.h:8:
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:684:30: error: static_cast from 'pointer' (aka 'std::__1::__list_node_base<AllowedArgs::HelpComponent, void *> *') to '__node_pointer' (aka 'std::__1::__list_node<AllowedArgs::HelpComponent, void *> *'), which are not related by inheritance, is not allowed
        __node_pointer __l = static_cast<__node_pointer>(
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:670:5: note: in instantiation of member function 'std::__1::__list_imp<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::clear' requested here
    clear();
    ^
/home/builder/TeamCity/work/6ff1207d8200ef46/depends/x86_64-apple-darwin11/native/bin/../include/c++/v1/list:780:29: note: in instantiation of member function 'std::__1::__list_imp<AllowedArgs::HelpComponent, std::__1::allocator<AllowedArgs::HelpComponent> >::~__list_imp' requested here
class _LIBCPP_TYPE_VIS_ONLY list
                            ^
2 errors generated.
Makefile:4308: recipe for target 'libbitcoin_util_a-util.o' failed
make[2]: *** [libbitcoin_util_a-util.o] Error 1
gcc gen_context.o -o gen_context

Compile on ARM;

allowed_args.cpp: In function 'void AllowedArgs::addZmqOptions(AllowedArgs::AllowedArgs&)':
allowed_args.cpp:328:9: error: expected unqualified-id before '.' token
         .addHeader(_("ZeroMQ notification options:"))
@zander

This comment has been minimized.

Show comment
Hide comment
@zander

zander Feb 15, 2017

I love the work you did, on first look its very clean code and well designed. Thanks!

zander commented Feb 15, 2017

I love the work you did, on first look its very clean code and well designed. Thanks!

@jamoes

This comment has been minimized.

Show comment
Hide comment
@jamoes

jamoes Feb 15, 2017

It looks like the linux issue was a syntax error in the ZMQ section. And, the MacOs build was just due to a missing #include <string> in allowed_args.h. I have pushed fixes for both issues.

jamoes commented Feb 15, 2017

It looks like the linux issue was a syntax error in the ZMQ section. And, the MacOs build was just due to a missing #include <string> in allowed_args.h. I have pushed fixes for both issues.

@zander

This comment has been minimized.

Show comment
Hide comment
@zander

zander Feb 15, 2017

Teamcity says all pass now :)

zander commented Feb 15, 2017

Teamcity says all pass now :)

@zander zander merged commit 3cca7f6 into bitcoinclassic:master Feb 18, 2017

@jamoes jamoes deleted the jamoes:allowed-args branch Feb 19, 2017

@kyuupichan kyuupichan referenced this pull request in BitcoinUnlimited/BitcoinUnlimited Apr 22, 2017

Merged

[Port] Command line option handling improvements from Classic #460

@kyuupichan

This comment has been minimized.

Show comment
Hide comment
@kyuupichan

kyuupichan Apr 23, 2017

This checkmempool line doesn't look right. It's requiredInt, but claims a default of false?

This checkmempool line doesn't look right. It's requiredInt, but claims a default of false?

This comment has been minimized.

Show comment
Hide comment
@jamoes

jamoes Apr 23, 2017

The -checkmempool argument value is expected to be an integer, so this would be more clear with '0' as the default instead of 'false'. I've made this change is #261. Thanks for catching this!

The -checkmempool argument value is expected to be an integer, so this would be more clear with '0' as the default instead of 'false'. I've made this change is #261. Thanks for catching this!

This comment has been minimized.

Show comment
Hide comment
@kyuupichan

kyuupichan Apr 23, 2017

I guess I'm also confused how a required value can have a default. I see optional integers too. What's the basic idea here?

I guess I'm also confused how a required value can have a default. I see optional integers too. What's the basic idea here?

This comment has been minimized.

Show comment
Hide comment
@jamoes

jamoes Apr 23, 2017

The default value in the help message only applies if the command-line flag is not specified at all. If the command-line flag is specified for a requiredInt, then it must have a value that is an int. For example: bitcoind -checkmempool is invalid, while bitcoind -checkmempool=10 is valid.

For the optionalBool types, the user can omit the value of the flag in order to indicate a value of true. For example: bitcoind -checkpoints is equivalent to bitcoind -checkpoints=true. optionalInt types are the same, but will convert to 0 if the value is omitted.

The default value in the help message only applies if the command-line flag is not specified at all. If the command-line flag is specified for a requiredInt, then it must have a value that is an int. For example: bitcoind -checkmempool is invalid, while bitcoind -checkmempool=10 is valid.

For the optionalBool types, the user can omit the value of the flag in order to indicate a value of true. For example: bitcoind -checkpoints is equivalent to bitcoind -checkpoints=true. optionalInt types are the same, but will convert to 0 if the value is omitted.

+#include "torcontrol.h"
+#include "txdb.h"
+#include "qt/guiconstants.h"
+#include "wallet/wallet.h"

This comment has been minimized.

@kyuupichan

kyuupichan Apr 23, 2017

FWIW the travis tests on BU showed this include needs to be surrounded with #ifdef ENABLE_WALLET.
Otherwise with --disable-wallet I got stuff like:

In file included from allowed_args.cpp:24:
In file included from ./wallet/wallet.h:18:
In file included from ./wallet/walletdb.h:11:
./wallet/db.h:22:10: fatal error: 'db_cxx.h' file not found
#include <db_cxx.h>
^
1 error generated.

Don't know if this happens on Classic.

@kyuupichan

kyuupichan Apr 23, 2017

FWIW the travis tests on BU showed this include needs to be surrounded with #ifdef ENABLE_WALLET.
Otherwise with --disable-wallet I got stuff like:

In file included from allowed_args.cpp:24:
In file included from ./wallet/wallet.h:18:
In file included from ./wallet/walletdb.h:11:
./wallet/db.h:22:10: fatal error: 'db_cxx.h' file not found
#include <db_cxx.h>
^
1 error generated.

Don't know if this happens on Classic.

@zander

This comment has been minimized.

Show comment
Hide comment
@zander

zander Apr 23, 2017

I didn't really investigate, just did a quick compile and that finished just fine (both 1.2 branch and master branch).

zander commented Apr 23, 2017

I didn't really investigate, just did a quick compile and that finished just fine (both 1.2 branch and master branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment