Skip to content

Fix for UB in getblocktemplate

Calin Culianu requested to merge cculianu/bitcoin-cash-node:fix_mr796_ub into master

Co-authored-by: Axel Gembe derago@gmail.com

Summary

This does code cleanup to fix the UB observed in issue #230 (closed). This bug was a regression introduced in !796 (merged).

The UB can happen with default values of nTimeLimit which lead to signed integer overflow. In order to fix the issue, the code has been switched over to using doubles for the time limit, and the fixed-point math has been changed to floating point math (which is easier to reason about). Also some code nits were added such as const-correcting a few things and more.

These UB issues can manifest themselves as crashes if building with -DCMAKE_BUILD_TYPE=Debug. I encourage reviewers to try building against master in that mode and watching the functional tests that touch getblocktemplate, such as mining_basic fail every time.

How to reproduce:

  1. Build master with -DCMAKE_BUILD_TYPE=Debug on Linux with gcc 8.x+
  2. Start bitcoind (testnet, testnet4, regtest, doesn't matter).
  3. Hit the rpc with getblocktemplate, see it crash with SIGABRT.

Explanation: GCC's debug mode traps on overflows, as a conveneince, that's why it SIGABRTS.

Screen_Shot_2021-01-11_at_2.47.52_PM

Screen_Shot_2021-01-11_at_2.47.23_PM

This commit fixes that.

Test Plan

  1. ninja all check-all

  2. Attempt to reproduce the bug in issue #230 (closed) (may require your platform raise SIGABRT on overflow, something that some debuggers do, or that some compile modes do).

  • For me this involved configuring with -DCMAKE_BUILD_TYPE=Debug on Debian 10 with GCC 8.3.0. Then none of the mining tests would work. Do this against master and it e.g. mining_basic should fail. Do it against this commit and it works again.
Edited by Calin Culianu

Merge request reports