Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rpc: signed-integer-overflow in analyzepsbt["estimated_feerate"] #27913

Closed
1 task done
maflcko opened this issue Jun 19, 2023 · 1 comment · Fixed by #27914
Closed
1 task done

rpc: signed-integer-overflow in analyzepsbt["estimated_feerate"] #27913

maflcko opened this issue Jun 19, 2023 · 1 comment · Fixed by #27914
Labels
Bug RPC/REST/ZMQ

Comments

@maflcko
Copy link
Member

maflcko commented Jun 19, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current behaviour

crash/UB in

bitcoin/src/rpc/rawtransaction.cpp

Line 1906 in f0758d8

result.pushKV("estimated_feerate", ValueFromAmount(psbta.estimated_feerate->GetFeePerK()));

Expected behaviour

no crash

Steps to reproduce

  • Compile with ubsan
  • UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/qt/bitcoin-qt
  • analyzepsbt cHNidP8BACkgICAgAAEgICAgIP8DABYgICAgICAgICAgICAgICAgICAgICAgICAgIAAA

Relevant log output

    #0 0x55a94d97befd in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63
    #1 0x55a94d4648ca in CFeeRate::GetFeePerK() const src/./policy/feerate.h:65:41
    #2 0x55a94d4648ca in analyzepsbt()::$_13::operator()(RPCHelpMan const&, JSONRPCRequest const&) const src/rpc/rawtransaction.cpp:1907:85
...
SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

current master

Operating system and version

Linux

Machine specifications

No response
@maflcko
Copy link
Member Author

maflcko commented Jun 19, 2023

See also https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59951

@achow101 achow101 mentioned this issue Jun 19, 2023
Merged
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jun 26, 2023
@fanquake @sidhujag
Merge bitcoin#27914: feerate: For GetFeePerK() return nSatoshisPerK i…
64e80e9 
…nstead of round trip through GetFee

11d6500 feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee (Andrew Chow)

Pull request description:

  Returning the sats/kvb does not need to round trip through GetFee(1000) since the feerate is already stored as sats/kvb.

  Fixes bitcoin#27913, although this does bring up a larger question of how we should handle such large feerates in fuzzing.

ACKs for top commit:
  furszy:
    Code ACK 11d6500

Tree-SHA512: bec1a0d4b572a0c810cf7eb4e97d729d67e96835c2d576a909f755b053a9707c2f1b3df9adb8f08a9c4d310cdbb8b1e1b42b9c004bd1ade02a07d8ce9e902138
@maflcko maflcko mentioned this issue Jun 30, 2023
Merged
fanquake added a commit to bitcoin-core/gui that referenced this issue Jul 7, 2023
@fanquake
Merge bitcoin/bitcoin#28015: fuzz: Generate rpc fuzz targets individu…
cf4da5e 
…ally

fa1e27f fuzz: Generate rpc fuzz targets individually (MarcoFalke)

Pull request description:

  The `rpc` fuzz target was added more than two years ago in e458631. However, the bug bitcoin/bitcoin#27913 was only found recently. Thus, it is pretty clear that fuzz engines can't deal with a search space that is too broad and can be extended in too many directions.

  Fix that by limiting the search space to each RPC method name and then iterate over all names, instead of letting the fuzz engine do the iteration.

  With this, the bug can be found in seconds, as opposed to years of CPU time (or never).

ACKs for top commit:
  brunoerg:
    ACK fa1e27f
  dergoegge:
    ACK fa1e27f

Tree-SHA512: 45ccba842367650d010320603153276b1b303deda9ba8c6bb31a4d2473b00aa5bca866db95f541485d65efd8276e2575026968c037872ef344fa33cf45bcdcd7
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jul 7, 2023
@fanquake @sidhujag
Merge bitcoin#28015: fuzz: Generate rpc fuzz targets individually
f6ab700 
fa1e27f fuzz: Generate rpc fuzz targets individually (MarcoFalke)

Pull request description:

  The `rpc` fuzz target was added more than two years ago in e458631. However, the bug bitcoin#27913 was only found recently. Thus, it is pretty clear that fuzz engines can't deal with a search space that is too broad and can be extended in too many directions.

  Fix that by limiting the search space to each RPC method name and then iterate over all names, instead of letting the fuzz engine do the iteration.

  With this, the bug can be found in seconds, as opposed to years of CPU time (or never).

ACKs for top commit:
  brunoerg:
    ACK fa1e27f
  dergoegge:
    ACK fa1e27f

Tree-SHA512: 45ccba842367650d010320603153276b1b303deda9ba8c6bb31a4d2473b00aa5bca866db95f541485d65efd8276e2575026968c037872ef344fa33cf45bcdcd7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee achow101/bitcoin
1 participant
@maflcko