[Qt] Ensure an item exists on the rpcconsole stack before adding #10060

1. 
   achow101 commented at 1:10 AM on March 23, 2017: member

   Ensures that there is an item on the rpcconsole stack before adding something to the current stack so that a segmentation fault does not occur.

   Currently Bitcoin Core will just crash if there happens to be a newline somewhere in the command followed by some non-whitespace characters which are followed by a space. This causes a segfault by attempting to get the current stack from the empty stack vector. This fix ensures that that current stack always exists so that the segfault does not happen.

   The crashing behavior can be tested by using this command in the rpcconsole:

   decoderawtransaction 01000000 03e7a6f3 a93af3ae 49518cbf 8600b799 b04a8b8a ae5145f0 b25df06e 
     175d472a a
   

   (copy and paste it as one line)

2. fanquake added the label Bug on Mar 23, 2017
3. fanquake added the label RPC/REST/ZMQ on Mar 23, 2017
4. fanquake added this to the milestone 0.14.1 on Mar 23, 2017
5. 
   fanquake commented at 1:43 AM on March 23, 2017: member

   Confirmed that Core does segfault when using the above command in the rpcconsole, and that it no longer happens with the PR'd fix.

6. in src/qt/rpcconsole.cpp:179 in f9c9ef74e0 outdated

    174 | @@ -175,6 +175,11 @@ bool RPCConsole::RPCParseCommandLine(std::string &strResult, const std::string &
    175 |              nDepthInsideSensitive = 1;
    176 |              filter_begin_pos = chpos;
    177 |          }
    178 | +        // Make sure stack is not empty before adding something
    179 | +        if(stack.empty()) 
   

   ---

   ---

   

   paveljanik commented at 6:43 AM on March 23, 2017:

   space after if please and left brace on the same line (or maybe one line if without braces - see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md).

7. 
   paveljanik commented at 6:44 AM on March 23, 2017: contributor

   Concept ACK

8. achow101 force-pushed on Mar 23, 2017
9. jonasschnelli added the label GUI on Mar 24, 2017
10. jonasschnelli removed the label RPC/REST/ZMQ on Mar 24, 2017
11. 
    jonasschnelli commented at 2:00 PM on March 24, 2017: contributor

    Thanks for pointing this out. Hmm... is there a reason to allow newlines in the command-line? I don't think so. Maybe this fix is more robust:

    diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
    index 7a0d0b3..c2f6434 100644
    --- a/src/qt/rpcconsole.cpp
    +++ b/src/qt/rpcconsole.cpp
    @@ -190,6 +190,7 @@ bool RPCConsole::RPCParseCommandLine(std::string &strResult, const std::string &
         };
     
         std::string strCommandTerminated = strCommand;
    +    std::replace(strCommandTerminated.begin(),strCommandTerminated.end(),'\n',' ');
         if (strCommandTerminated.back() != '\n')
             strCommandTerminated += "\n";
         for (chpos = 0; chpos < strCommandTerminated.size(); ++chpos)
    

12. achow101 renamed this:
    [RPC] Ensure an item exists on the rpcconsole stack before adding
    [Qt] Ensure an item exists on the rpcconsole stack before adding
    on Mar 24, 2017
13. 
    achow101 commented at 3:10 PM on March 24, 2017: member

    @jonasschnelli The segfault bug happens with more than just a newline in the command. It also happens if an end parenthesis ) is also in the command without a start parenthesis ( before it.

14. in src/qt/rpcconsole.cpp:179 in f3fa05a96d outdated

     174 | @@ -175,6 +175,10 @@ bool RPCConsole::RPCParseCommandLine(std::string &strResult, const std::string &
     175 |              nDepthInsideSensitive = 1;
     176 |              filter_begin_pos = chpos;
     177 |          }
     178 | +        // Make sure stack is not empty before adding something
     179 | +        if(stack.empty()) {
    

    ---

    ---

    

    sipa commented at 8:40 PM on March 24, 2017:

    Nit: space after if please.

15. 
    sipa commented at 8:40 PM on March 24, 2017: member

    utACK, with nit.

16. Ensure an item exists on the rpcconsole stack before adding

    Ensures that there is an item on the rpcconsole stack before adding something to the current stack so that a segmentation fault does not occur.

    4df76e270c
17. achow101 force-pushed on Mar 25, 2017
18. 
    jonasschnelli commented at 7:54 AM on March 27, 2017: contributor

    Thanks @achow101 for clarification. Tested ACK 4df76e270caa9d828179cae1c7a8918d6f91ec21

19. jonasschnelli added the label Needs backport on Mar 27, 2017
20. 
    jonasschnelli commented at 7:55 AM on March 27, 2017: contributor

    Needs backport.

21. jonasschnelli merged this on Mar 27, 2017
22. jonasschnelli closed this on Mar 27, 2017

    ---

23. jonasschnelli referenced this in commit 0ddea4430d on Mar 27, 2017
24. MarcoFalke referenced this in commit ddc2dd1612 on Mar 27, 2017
25. MarcoFalke removed the label Needs backport on Mar 27, 2017
26. achow101 deleted the branch on Apr 5, 2017
27. codablock referenced this in commit b018751852 on Jan 26, 2018
28. andvgal referenced this in commit 17d7949083 on Jan 6, 2019
29. CryptoCentric referenced this in commit 343954b088 on Feb 27, 2019
30. DrahtBot locked this on Sep 8, 2021

Contributors

achow101fanquakepaveljanikjonasschnellisipa

Milestone
0.14.1