[init] Check non-emptiness of -blocknotify command prior to executing #10939

practicalswift commented at 11:14 PM on July 26, 2017: contributor

Check that -blocknotify command is non-empty before executing.

To make the BlockNotifyCallback(...) (-blocknotify) behaviour consistent with that of:

AlertNotify(...) (-alertnotify)
AddToWallet(...) (-walletnotify)

practicalswift renamed this:
~~Check that -blocknotify command is non-empty before executing~~
[init] Check that -blocknotify command is non-empty before executing
on Jul 26, 2017

practicalswift renamed this:
~~[init] Check that -blocknotify command is non-empty before executing~~
[init] Check non-emptiness of -blocknotify command before executing
on Jul 26, 2017

practicalswift renamed this:
~~[init] Check non-emptiness of -blocknotify command before executing~~
[init] Check non-emptiness of -blocknotify command prior to executing
on Jul 26, 2017

in src/wallet/wallet.cpp:991 in 7f1ba9ae3b outdated

 939 | @@ -940,7 +940,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 940 |      // notify an external script when a wallet transaction comes in or is updated
 941 |      std::string strCmd = GetArg("-walletnotify", "");
 942 |  
 943 | -    if ( !strCmd.empty())
 944 | +    if (!strCmd.empty())

promag commented at 12:49 AM on July 27, 2017:

Unrelated.

practicalswift commented at 7:26 AM on July 27, 2017:

That change was intentional to show how the logic is handled in AddToWallet (-walletnotify).

in src/init.cpp:550 in 7f1ba9ae3b outdated

 544 | @@ -545,9 +545,10 @@ static void BlockNotifyCallback(bool initialSync, const CBlockIndex *pBlockIndex
 545 |          return;
 546 |  
 547 |      std::string strCmd = GetArg("-blocknotify", "");
 548 | -
 549 | -    boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex());
 550 | -    boost::thread t(runCommand, strCmd); // thread runs free
 551 | +    if (!strCmd.empty()) {

promag commented at 12:50 AM on July 27, 2017:

No need to test because in init.cpp there's:

    if (IsArgSet("-blocknotify"))
        uiInterface.NotifyBlockTip.connect(BlockNotifyCallback);

practicalswift commented at 7:27 AM on July 27, 2017:

Does IsArgSet(…) test for emptiness?

promag commented at 8:26 AM on July 27, 2017:

Ignore if you want to keep the test here.

promag commented at 12:53 AM on July 27, 2017: member

Partial ACK. IMO the check should be in init.cpp:

if (!GetArg("-blocknotify", "").empty()) {
    uiInterface.NotifyBlockTip.connect(BlockNotifyCallback);
}

That way it's checked one time only.

practicalswift commented at 7:35 AM on July 27, 2017: contributor

@promag I prefer doing a check inside of BlockNotifyCallback(…) (as close as possible to the RunCommand(…)) since:

BlockNotifyCallback(…) might be invoked from other places in the future.
That is consistent with how it is done for AlertNotify(...) (-alertnotify) and AddToWallet(...) (-walletnotify).

promag commented at 8:27 AM on July 27, 2017: member

Side note, should we treat empty args as invalid?

practicalswift commented at 8:46 AM on July 27, 2017: contributor

@promag In the specific case of -{alert,block,wallet}notify: yes - no need to execute "" :-)

laanwj commented at 9:07 AM on July 27, 2017: member

Side note, should we treat empty args as invalid?

Depends on the specific argument. For some things, an empty string might be a valid value, for others it's not.

For executing a command it makes sense to skip it when the string is empty. What does system("") do? spawn a shell?

BTW: Why not handle this in runCommand? that seems the shortest path to making this consistent.

laanwj commented at 9:09 AM on July 27, 2017: member

What does system("") do? spawn a shell?

It already does nothing! Same for system(" "); etc. Why do we need this change?

promag commented at 11:09 AM on July 27, 2017: member

@laanwj the only reason to test "" in each is to add some custom logging. Otherwise agree with you (despite the fact that we can avoid connecting to the signal in init).

laanwj commented at 11:38 AM on July 27, 2017: member

Not convinced that "custom logging" makes sense here. The user provided an empty command, which non-ambiguously means they don't want it to do anything. It doesn't do anything.

(maybe skipping could be considered a small optimization, but could leave it up to the user to just not provide the argument at all so that the signal doesn't get registered? I have a hard time imagining any circumstance where this would matter at all)

practicalswift commented at 12:12 PM on July 27, 2017: contributor

@laanwj My original idea was to make -blocknotify, -alertnotify and -walletnotify behave identically with regards to thread creation and execve(…)-calls in case of an "" argument. Not a big issue, just a small step towards increased user interface consistency :-)

promag commented at 3:30 PM on July 31, 2017: member

@laanwj IMO empty commands should be invalid. The same goes for when a command does not exists. Not providing the argument/command should be the correct way to run nothing.

For instance, with find it fails:

$ find . -exec \;
find: -exec: no command specified

Check that -blocknotify command is non-empty before executing

To make BlockNotifyCallback(...) (-blocknotify) consistent with:
* AlertNotify(...) (-alertnotify)
* AddToWallet(...) (-walletnotify)

6fb8f5f17c

Skip sys::system(...) call in case of empty command cffe85f975

practicalswift force-pushed on Aug 14, 2017

jnewbery commented at 9:10 PM on September 1, 2017: member

(slightly offtopic) @promag - I'm with you on this one. I'd like there to be better systematic validation of command line-arguments.

I think you've spoken about argument validation on a few issues. Perhaps we should raise this in a future meeting to get concept feedback?

promag commented at 9:36 PM on September 1, 2017: member

Yes @jnewbery, IMO it can be improved. We should validate command line arguments in the same way we validate RPC arguments. Again, for me "" is an invalid command.

luke-jr approved

luke-jr commented at 6:39 AM on September 2, 2017: member

utACK

luke-jr commented at 6:42 AM on September 2, 2017: member

We shouldn't make the empty command invalid so quickly: if one has blocknotify=foobar in the bitcoin.conf file, they may wish to use -blocknotify= on the commandline to temporarily disable it.

(The alternative, -noblocknotify, is probably broken...)

promag commented at 8:37 AM on September 2, 2017: member

they may wish to use -blocknotify=

They can remove the line or comment it out?

luke-jr commented at 8:42 AM on September 2, 2017: member

Better if no change to the config file is needed for such a temporary purpose.

jnewbery commented at 4:00 PM on September 4, 2017: member

@luke-jr raises a good point. Being able to override and temporarily disable .conf file options from the command line is very useful. This PR allows that (supplying blocknotify= won't cause an error).

Has anyone tested what happens with noblocknotify?

laanwj commented at 12:53 PM on October 4, 2017: member

We shouldn't make the empty command invalid so quickly: if one has blocknotify=foobar in the bitcoin.conf file, they may wish to use -blocknotify= on the commandline to temporarily disable it.

Good point! And the first good argument for doing this at all. Being able to overriding the blocknotify command with "" (say, from the command line) to disable it is useful. In that case you don't want an empty command to be executed as that just wastes resources.

utACK cffe85f

laanwj merged this on Oct 4, 2017

laanwj closed this on Oct 4, 2017

laanwj referenced this in commit a1f7f18709 on Oct 4, 2017

PastaPastaPasta referenced this in commit f3e6525f2d on Jan 31, 2020

PastaPastaPasta referenced this in commit 7eb8b96436 on Jan 31, 2020

PastaPastaPasta referenced this in commit 4aca713990 on Feb 4, 2020

PastaPastaPasta referenced this in commit 09184fc89c on Feb 9, 2020

ckti referenced this in commit cc3dbc03df on Mar 28, 2021

practicalswift deleted the branch on Apr 10, 2021

gades referenced this in commit 0f5fd82514 on Feb 15, 2022

DrahtBot locked this on Aug 18, 2022