Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) by practicalswift · Pull Request #14242 · bitcoin/bitcoin

practicalswift · 2018-09-17T14:33:30Z

Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...).

Background reading: memcpy (and friends) with NULL pointers

Steps to reproduce:

./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py

practicalswift · 2018-09-17T14:34:52Z

Introduced in PR #11372 (119b0f8). Friendly ping @sipa – would you mind reviewing? :-)

maflcko · 2018-09-17T16:30:08Z

Shouldn't memory_cleanse be more robust in handling zero-length data?

practicalswift · 2018-09-17T19:16:59Z

@MarcoFalke Sure! Added a commit. Please re-review :-)

src/key_io.cpp

promag · 2018-09-18T00:15:09Z

Shouldn't memory_cleanse be more robust in handling zero-length data?

@MarcoFalke the same for memset?

-0 ae17de5 because it changes memory_cleanse invariant.

IMO should avoid the call to memory_cleanse.

practicalswift · 2018-09-18T04:35:13Z

@MarcoFalke I think @promag has a good point. Now reverted to the original version.

practicalswift · 2018-09-18T04:35:41Z

@promag Thanks for reviewing. Now updated. Please re-review :-)

…defined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * #14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * #14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * #13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue #14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4

maflcko · 2018-11-05T20:11:27Z

Could remove the suppression from the file in contrib?

practicalswift · 2018-11-06T23:26:18Z

@MarcoFalke Good point! Added commit 1cab4bb7e09c9d68e104460b67e5804848cd3c2e which removes UBSan suppression.

Please re-review :-)

DrahtBot · 2018-11-08T21:48:56Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14239 (Avoid dividing by zero (undefined behaviour) in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) by practicalswift)

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.

practicalswift · 2018-11-23T15:58:25Z

Rebased!

src/key_io.cpp

practicalswift · 2019-01-05T16:03:55Z

@luke-jr @promag @MarcoFalke Would you mind re-reviewing? :-)

practicalswift · 2019-01-15T19:58:14Z

@MarcoFalke Could this one get a release milestone? :-)

src/key_io.cpp

promag · 2019-02-07T15:19:06Z

Agree with @laanwj, also (nit) could squash.

utACK 524a876 otherwise.

…an invalid string is passed to DecodeSecret(...)

practicalswift · 2019-02-07T21:31:07Z

@laanwj @promag Good points. Fixed and squashed. Please re-review :-)

promag

utACK d855e4c.

…tr, 0, 0)) if an invalid string is passed to DecodeSecret(...) d855e4c Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) (practicalswift) Pull request description: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`. Background reading: [memcpy (and friends) with NULL pointers](https://www.imperialviolet.org/2016/06/26/nonnull.html) Steps to reproduce: ``` ./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py ``` Tree-SHA512: b8325ced4f724d9c03065e0747af56b1f297a90d9fb09a24d46c3231a90dce3df6299f2c41f863b5cec18eaeded7b46ee4b93d9a52adc2541eb4c44d2c0965d9

…an invalid string is passed to DecodeSecret(...) Summary: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`. Backport of Bitcoin Core PR14242 bitcoin/bitcoin#14242 Test Plan: ``` make check ``` Reviewers: Fabien, #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4017

…t(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) d855e4c Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) (practicalswift) Pull request description: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`. Background reading: [memcpy (and friends) with NULL pointers](https://www.imperialviolet.org/2016/06/26/nonnull.html) Steps to reproduce: ``` ./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py ``` Tree-SHA512: b8325ced4f724d9c03065e0747af56b1f297a90d9fb09a24d46c3231a90dce3df6299f2c41f863b5cec18eaeded7b46ee4b93d9a52adc2541eb4c44d2c0965d9

… the undefined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * bitcoin#14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * bitcoin#14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * bitcoin#13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue bitcoin#14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4

… the undefined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * bitcoin#14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * bitcoin#14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * bitcoin#13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue bitcoin#14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4 continued 14252 Co-authored-by: UdjinM6 <[email protected]>

…t(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) d855e4c Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) (practicalswift) Pull request description: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`. Background reading: [memcpy (and friends) with NULL pointers](https://www.imperialviolet.org/2016/06/26/nonnull.html) Steps to reproduce: ``` ./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py ``` Tree-SHA512: b8325ced4f724d9c03065e0747af56b1f297a90d9fb09a24d46c3231a90dce3df6299f2c41f863b5cec18eaeded7b46ee4b93d9a52adc2541eb4c44d2c0965d9

Backports v0.18: PR's bitcoin#14242, 15351 and 14519

…t(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) d855e4c Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) (practicalswift) Pull request description: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`. Background reading: [memcpy (and friends) with NULL pointers](https://www.imperialviolet.org/2016/06/26/nonnull.html) Steps to reproduce: ``` ./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py ``` Tree-SHA512: b8325ced4f724d9c03065e0747af56b1f297a90d9fb09a24d46c3231a90dce3df6299f2c41f863b5cec18eaeded7b46ee4b93d9a52adc2541eb4c44d2c0965d9

… the undefined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * bitcoin#14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * bitcoin#14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * bitcoin#13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue bitcoin#14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4 continued 14252 Co-authored-by: UdjinM6 <[email protected]>

maflcko reviewed Sep 17, 2018

View reviewed changes

src/key_io.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the ub-in-DecodeSecret branch from ae17de5 to 8ea1d7e Compare September 18, 2018 04:34

This was referenced Sep 18, 2018

build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) #14252

Merged

functional tests fail --with-sanitizers=undefined #14059

Closed

bitcoin deleted a comment from DrahtBot Nov 5, 2018

practicalswift force-pushed the ub-in-DecodeSecret branch from 8ea1d7e to 1cab4bb Compare November 6, 2018 23:25

practicalswift mentioned this pull request Nov 16, 2018

fix an undefined behavior in uint::SetHex #14734

Merged

DrahtBot added the Needs rebase label Nov 23, 2018

practicalswift force-pushed the ub-in-DecodeSecret branch from 1cab4bb to f094aeb Compare November 23, 2018 15:58

DrahtBot removed the Needs rebase label Nov 23, 2018

luke-jr reviewed Dec 19, 2018

View reviewed changes

src/key_io.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the ub-in-DecodeSecret branch from f094aeb to 524a876 Compare January 5, 2019 16:00

maflcko added this to the 0.18.0 milestone Jan 15, 2019

laanwj reviewed Feb 6, 2019

View reviewed changes

src/key_io.cpp Outdated Show resolved Hide resolved

laanwj added the Refactoring label Feb 6, 2019

Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if …

d855e4c

…an invalid string is passed to DecodeSecret(...)

practicalswift force-pushed the ub-in-DecodeSecret branch from 524a876 to d855e4c Compare February 7, 2019 21:30

promag reviewed Feb 7, 2019

View reviewed changes

laanwj merged commit d855e4c into bitcoin:master Feb 8, 2019

practicalswift deleted the ub-in-DecodeSecret branch April 10, 2021 19:37

pravblockc mentioned this pull request Aug 10, 2021

Backports v0.18: PR's #14242, 15351 and 14519 dashpay/dash#4329

Merged

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Aug 21, 2021

Merge pull request #4329 from pravblockc/backports-v0.18-pr9

c469721

Backports v0.18: PR's bitcoin#14242, 15351 and 14519

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

practicalswift commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 17, 2018

Uh oh!

practicalswift commented Sep 17, 2018

Uh oh!

Uh oh!

promag commented Sep 18, 2018

Uh oh!

practicalswift commented Sep 18, 2018

Uh oh!

practicalswift commented Sep 18, 2018

Uh oh!

maflcko commented Nov 5, 2018

Uh oh!

practicalswift commented Nov 6, 2018

Uh oh!

DrahtBot commented Nov 8, 2018

Conflicts

Uh oh!

practicalswift commented Nov 23, 2018

Uh oh!

Uh oh!

practicalswift commented Jan 5, 2019

Uh oh!

practicalswift commented Jan 15, 2019

Uh oh!

Uh oh!

promag commented Feb 7, 2019

Uh oh!

practicalswift commented Feb 7, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Sep 17, 2018 •

edited

Loading

practicalswift commented Sep 17, 2018 •

edited

Loading