bugfix: rest: avoid segfault for invalid URI #27468

1. 
   pablomartin4btc commented at 11:48 pm on April 14, 2023: member

   Minimal fix to get it promptly into 25.0 release (suggested by stickies-v and supported by vasild )

   Please check #27253 for reviewers comments and acks regarding this PR and read the commit comment message body for more details about the fix.

2. 
   DrahtBot commented at 11:48 pm on April 14, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	stickies-v, achow101
   Stale ACK	vasild

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #27253 (httpserver, rest: fix segmentation fault on evhttp_uri_get_query by pablomartin4btc)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. fanquake added this to the milestone 25.0 on Apr 15, 2023
4. fanquake requested review from vasild on Apr 15, 2023
5. fanquake requested review from stickies-v on Apr 15, 2023
6. stickies-v approved
7. 
   stickies-v commented at 12:52 pm on April 15, 2023: contributor

   ACK 827b14c33f39131ede35ddecde75cc052d977ec5
8. theStack changes_requested
9. 
   theStack commented at 4:19 pm on April 16, 2023: contributor

   It’s still possible to provoke a segfault via the /rest/mempool endpoint, which also uses GetQueryParameter() (tested on a bitcoind -signet -rest instance):

   0$ curl localhost:38332/rest/mempool/contents.json?%
   1curl: (52) Empty reply from server
   2
   3bitcoind output:
   4libc++abi: terminating with uncaught exception of type std::runtime_error: URI parsing failed, it likely contained RFC 3986 invalid characters
   5Abort trap (core dumped)
   

   Should be easy to fix by catching std::runtime_error also at the proper places within rest_mempool.

10. 
    pablomartin4btc commented at 0:03 am on April 17, 2023: member

    It’s still possible to provoke a segfault via the /rest/mempool endpoint, which also uses GetQueryParameter() (tested on a bitcoind -signet -rest instance):

    Thanks @theStack. I’ll fix it asap.

11. vasild approved
12. 
    vasild commented at 7:16 am on April 17, 2023: contributor

    ACK 827b14c33f39131ede35ddecde75cc052d977ec5
13. bugfix: rest: avoid segfault for invalid URI

    `evhttp_uri_parse` can return a nullptr, for example when the URI
    contains invalid characters (e.g. "%").
    `GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
    straight into `evhttp_uri_get_query`, which means that anyone calling
    a REST endpoint in which query parameters are used (e.g. `rest_headers`)
    can cause a segfault.
    
    This bugfix is designed to be minimal and without additional behaviour change.
    Follow-up work should be done to resolve this in a more general and robust way,
    so not every endpoint has to handle it individually.

    11422cc572
14. pablomartin4btc force-pushed on Apr 17, 2023
15. 
    pablomartin4btc commented at 1:16 pm on April 17, 2023: member

    Updated changes:

    • Fixed issue detected by @theStack on the rest/mempool endpoint that was also calling GetQueryParameter().
16. fanquake requested review from stickies-v on Apr 17, 2023
17. fanquake requested review from vasild on Apr 17, 2023
18. fanquake requested review from theStack on Apr 17, 2023
19. stickies-v approved
20. 
    stickies-v commented at 1:48 pm on April 17, 2023: contributor

    re-ACK 11422cc
21. 
    achow101 commented at 1:51 pm on April 17, 2023: member

    ACK 11422cc5720c8d73a87600de8fe8abb156db80dc
22. fanquake merged this on Apr 17, 2023
23. fanquake closed this on Apr 17, 2023

    ---

24. 
    theStack commented at 2:31 pm on April 17, 2023: contributor

    post-merge ACK 11422cc5720c8d73a87600de8fe8abb156db80dc
25. sidhujag referenced this in commit 200330a67d on Apr 17, 2023
26. theStack referenced this in commit 6a77d290da on Apr 17, 2023
27. fanquake referenced this in commit 467fa89438 on Apr 18, 2023
28. fanquake referenced this in commit 3a26b19df2 on Apr 18, 2023
29. fanquake referenced this in commit 15a24781d0 on Apr 18, 2023
30. sidhujag referenced this in commit 21e773bf32 on Apr 18, 2023
31. RandyMcMillan referenced this in commit b027eedcb1 on May 27, 2023
32. bitcoin locked this on Apr 16, 2024

Contributors
pablomartin4btc DrahtBot stickies-v theStack vasild achow101

Review Requested
vasild

Milestone
25.0