base58: add paranoid return value checks #4337

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:reallycheck changing 1 files +4 −5

jgarzik commented at 7:54 PM on June 13, 2014: contributor

Always clear buffers and check return values.
base58: add paranoid return value checks 88df548dde
sipa commented at 8:04 PM on June 13, 2014: member

No objection to the patch as it makes the code more obviously correct, but DecodeBase58 never touches its vch reference in case it returns false, which for the DecodeBase58Check call in SetString means that vchTemp will always be empty in the case it fails. Or in other words, the (vchTemp.size() < nVersionBytes) check will always trigger if SetString's call to DecodeBase58Check fails.
BitcoinPullTester commented at 8:11 PM on June 13, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/88df548dde280efac8eb33520f8192885455bc03 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
jgarzik commented at 8:16 PM on June 13, 2014: contributor

@sipa Yes that ensures IsValid() will return false after CBitcoinAddress("carefully constructed garbage") It is possible but unlikely that someone directly using DecodeBase58Check() may have been impacted by the lack of buffer clearing in one error case.
in src/base58.cpp:None in 88df548dde
```
 152 | @@ -154,8 +153,8 @@ void CBase58Data::SetData(const std::vector<unsigned char> &vchVersionIn, const
 153 |  
 154 |  bool CBase58Data::SetString(const char* psz, unsigned int nVersionBytes) {
 155 |      std::vector<unsigned char> vchTemp;
 156 | -    DecodeBase58Check(psz, vchTemp);
 157 | -    if (vchTemp.size() < nVersionBytes) {
 158 | +    bool rc58 = DecodeBase58Check(psz, vchTemp);
 159 | +    if ((!rc58) || (vchTemp.size() < nVersionBytes)) {
```
sipa commented at 8:39 PM on June 13, 2014:

Is the intermediary rc58 variable necessary here?

jgarzik commented at 8:49 PM on June 13, 2014:

Necessary? No. But IMO it makes things slightly more readable, while producing the same compiler-optimized end result as alternatives. The compiler generates a temporary for the direct result of each function/method call, in any case.

sipa commented at 8:55 PM on June 13, 2014:

Oh, not a comment on performance. I just find it more readable to write it in the condition directly. Just nitpicking, feel free to ignore.

laanwj commented at 5:47 AM on June 14, 2014:

If there is a function with an out parameter, checking that result in the same if() clause can be harder to read. Of course the || evaluation order makes sure that this is correct, but I prefer logical conditions that don't depend on the order, and thus don't break when they are rearranged. (so TL;DR I'm ok with the way it is now)
laanwj commented at 5:48 AM on June 14, 2014: member

ACK
sipa commented at 10:16 AM on June 16, 2014: member

Untested ACK
laanwj merged this on Jun 16, 2014
laanwj closed this on Jun 16, 2014
laanwj referenced this in commit f73082db87 on Jun 16, 2014
jgarzik deleted the branch on Aug 24, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors