Merge #14734: fix an undefined behavior in uint::SetHex
ClosedPublic
Actions

Authored by jasonbcox on Jul 7 2020, 18:48.

Details

Reviewers

majcosta

Group Reviewers

Restricted Project

Commits

rABC63a5cddd312d: Merge #14734: fix an undefined behavior in uint::SetHex

Summary

0f459d868d85053f1cc066ea9099793f88cbd655 fix an undefined behavior in uint::SetHex (Kaz Wesley)

Pull request description:

Decrementing psz beyond the beginning of the string is UB, even though
the out-of-bounds pointer is never dereferenced.

I don't think any clang sanitizer covers this, so I don't see any way a test could catch the original behavior.

ACKs for top commit:

promag:
  utACK 0f459d8.
l2a5b1:
  utACK 0f459d868d85053f1cc066ea9099793f88cbd655

Tree-SHA512: 388223254ea6e955f643d2ebdf74d15a3d494e9f0597d9f05987ebb708d7a1cc06ce64bd25d447d75b5f5561bdae9630dcf25adb7bd75f7a382298b95d127162

Backport of Core PR14734

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr14734

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 11888
Build 23508: Default Diff Build & Tests	Link to the build
Build 23507: Build without Wallet	Link to the build
Build 23506: Build with clang-10	Link to the build
Build 23505: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jul 7 2020, 18:48

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 7 2020, 18:48

jasonbcox requested review of this revision.Jul 7 2020, 18:48

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Harbormaster completed remote builds in B11887: Diff 22056.Jul 7 2020, 19:03

majcosta accepted this revision.Jul 7 2020, 19:18

This revision is now accepted and ready to land.Jul 7 2020, 19:18

majcosta requested changes to this revision.Jul 7 2020, 19:20

majcosta added inline comments.

src/uint256.cpp
45 ↗	(On Diff #22056)	Does this change behavior? Should be uint8_t(::HexDigit(psz[--digits])) << 4

This revision now requires changes to proceed.Jul 7 2020, 19:20

jasonbcox added inline comments.Jul 7 2020, 21:51

src/uint256.cpp
45 ↗	(On Diff #22056)	I'll fix it to be completely in line with Core's code, but no it doesn't change behavior. The largest return value of HexDigit() is 15 (15 << 4 = 15 * 4 = 60). Fits just fine in a char.