qt: fix out-of-bounds read in RPCParseCommandLine on empty command #944

nabhan06 commented at 9:34 AM on July 3, 2026: none

When a console line has no command name (it starts with ), or is (), (, or ,), RPCParseCommandLine reaches the command-execution branch while the current argument frame is still empty, so stack.back()[0] reads out of bounds and the argument list built from stack.back().begin() + 1 to end() is an invalid iterator range (throws std::length_error in practice, UBSan flags the null-pointer reference otherwise).

The ( branch already guards the frame with stack.back().size() > 0, so I add the same check to the )/newline branch and the empty frame is skipped. To be clear, ( alone isn't safe on master either: it fails via the \n branch, not via the ( branch itself (the state there isn't STATE_ARGUMENT), and , alone fails the same way.

Since there's no command to run in any of these cases, the parser now returns false so the console reports an invalid command line, consistent with other fully-invalid input like a bare ' or ", rather than silently ignoring it.

Regression cases added to rpcNestedTests for ), (), ( and , (all abort on master without the guard), plus getblockchaininfo) which stays tolerated.

DrahtBot commented at 9:34 AM on July 3, 2026: contributor

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

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	RandyMcMillan, pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

RandyMcMillan commented at 3:19 PM on July 3, 2026: contributor

Concept ACK

in src/qt/rpcconsole.cpp:293 in fb98a78035 outdated

 289 | @@ -290,7 +290,7 @@ bool RPCConsole::RPCParseCommandLine(interfaces::Node* node, std::string &strRes
 290 |                          curarg.clear();
 291 |                          state = STATE_EATING_SPACES_IN_BRACKETS;
 292 |                      }
 293 | -                    if ((ch == ')' || ch == '\n') && stack.size() > 0)
 294 | +                    if ((ch == ')' || ch == '\n') && stack.size() > 0 && stack.back().size() > 0)

pablomartin4btc commented at 1:26 AM on July 13, 2026:

The guard is the right approach and the scope is correct: both ) and \n need it since ) alone / () fail at the ) check before the appended \n is ever reached, while ( alone and , alone fail only via the \n path.

in src/qt/test/rpcnestedtests.cpp:138 in fb98a78035

 133 | @@ -134,6 +134,8 @@ void RPCNestedTests::rpcNestedTests()
 134 |      QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo() getblockchaininfo()"), std::runtime_error); //invalid syntax
 135 |      RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo("); //tolerate non closing brackets if we have no arguments
 136 |      RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()()()"); //tolerate non command brackets
 137 | +    RPCConsole::RPCExecuteCommandLine(m_node, result, ")"); //tolerate a closing bracket with no command (empty argument stack)
 138 | +    RPCConsole::RPCExecuteCommandLine(m_node, result, "()"); //tolerate empty brackets with no command

pablomartin4btc commented at 1:26 AM on July 13, 2026:

Missing some regression cases. Two additional inputs fail on master with the same OOB but aren't covered by the PR's two new tests:

RPCConsole::RPCExecuteCommandLine(m_node, result, "("); // fails on master via \n branch, fixed by this PR
RPCConsole::RPCExecuteCommandLine(m_node, result, ","); // same

This test doesn't fail but it's also missing:

RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo)”); //tolerate a closing bracket with no argument stack

nabhan06 commented at 7:23 AM on July 13, 2026:

Added all three: ( and , now have their own cases (both abort on master via the \n branch), and getblockchaininfo) as a tolerated case. Since these no-command inputs now return false, the )/()/(/, cases assert that with QVERIFY.

pablomartin4btc commented at 1:53 AM on July 13, 2026: contributor

Concept ACK on the fix — Error: vector is a raw internal exception message with no meaning to the user.

I think the PR description is slightly misleading. It says "The ( branch already guards this with stack.back().size() > 0", implying ( alone is safe on master. It isn't — ( alone does fail on master via the \n branch, just not via the ( branch itself (state != STATE_ARGUMENT). Worth clarifying.

Tested at fb98a780355419aa5942b09f3cc4bc5342d714bd.

Behaviour suggestion — rather than silently ignoring these inputs as proposed, I'd prefer returning false so the UI shows "Invalid command line" popup, consistent with other fully-invalid input (e.g. bare ' or ") as there is no command name to speak of in these cases either. Silent ignore feels inconsistent with existing error handling.

Left a couple of comments.

qt: fix out-of-bounds read in RPCParseCommandLine on empty command

When a console line has no command name (e.g. ")", "()", "(", ",") the
parser reaches the command-execution branch with an empty argument frame,
so stack.back()[0] reads out of bounds and the argument list built from
stack.back().begin() + 1 is an invalid iterator range. Guard the ')' and
newline branch with stack.back().size() > 0, matching the existing '('
branch, so the empty frame is skipped.

Since there is no command to run in these cases, return false so the UI
reports an invalid command line, consistent with other fully-invalid input
like a bare quote, instead of silently ignoring it.

Add regression cases to rpcNestedTests for the inputs that abort without
the guard.

fef99e6563

nabhan06 force-pushed on Jul 13, 2026

nabhan06 commented at 7:23 AM on July 13, 2026: none

Good points, both addressed.

Switched the no-command inputs (), (), (, ,) to return false instead of silently ignoring them, so the console shows an invalid command line like it does for a bare ' or ". A command_parsed flag tracks whether the execution branch ever ran, and the final STATE_EATING_SPACES returns it. The size guard stays since it's what keeps the OOB read from happening in the first place.

Also fixed the description: ( alone does fail on master, via the \n branch rather than the ( branch itself. Rewrote that paragraph and added the extra regression cases. rpcNestedTests is green.