Fixes subscript 0 (&var[0]) where should use (var.data()) instead.

[JeremyRubin]

This PR fixes up a couple places where the codebase has semi-risky uses of 0 subscript. I don't think any of these are actively bugs, but some of them are unsafe (or unexpected-errors) for unsanitized inputs (such as in addrdb or key.cpp).

This also allows us to eliminate some bounds checks, so I guess this is a net-negative patch. I don't think any of those bounds checks are performance sensitive, but worth double checking in review.

p.s. I also fix some confusing code in murmurhash (with non obvious offset-math) as it took me a little bit to see that it was correct, could separate that out if desired.

[dcousens]

std::string maybe?

[JeremyRubin]

Sorry for the bit of line noise, there was some UB when I removed some of the branches (memset/memcpy/read/write 0 bytes on nullptr/invalid ptr) that was causing one of the builds (with more aggressive runtime checks) to fail. I added them back in and all seems well now.

[laanwj]

Whoa, good catch.

I'm shocked that &vch[0] use was still so rampant in the code. We'd introduced begin_ptr with the intention to replace those, and phased even that out again with C++11 .data().

Anyhow, concept ACK.

[laanwj]

I wonder why we're casting to (char*) here iso (const char*), even though these are write operations and the type passed in is const.

There's a couple more of these. Though seems out of scope to fix in this pull.

[JeremyRubin]

Yeah that seems obviously incorrect, but I think I understand why it was done (or at least have a theory). Because the methods are all generic w.r.t. the stream type, they probably are made to work with some arbitrary stream that can't accept a const char *. Hopefully not the case anymore...

Anyways, probably best to fix those up separately as you suggest.

[laanwj]

Sure, but it would be wrong for a stream write to take a char* i.s.o. const char*, so fixing those just means the const-correctness spreads.

[JeremyRubin]

There are still a few more places that were trickier/not possible to correct (where the type passed in is templated, such that it would be a pointer or a vector).

I also didn't fix anything in subdirectories, because I felt this PR was getting a bit large already, and there are a few you can still grep for pretty easily there:

❯ grep '\&v[A-z0-9_]*\[0\]' */*.cpp | nl
    1	qt/signverifymessagedialog.cpp:    ui->signatureOut_SM->setText(QString::fromStdString(EncodeBase64(&vchSig[0], vchSig.size())));
    2	rpc/misc.cpp:    return EncodeBase64(&vchSig[0], vchSig.size());
    3	script/ismine.cpp:        CRIPEMD160().Write(&vSolutions[0][0], vSolutions[0].size()).Finalize(hash.begin());
    4	script/sigcache.cpp:        CSHA256().Write(nonce.begin(), 32).Write(hash.begin(), 32).Write(&pubkey[0], pubkey.size()).Write(&vchSig[0], vchSig.size()).Finalize(entry.begin());
    5	script/sign.cpp:        CRIPEMD160().Write(&vSolutions[0][0], vSolutions[0].size()).Finalize(h160.begin());
    6	script/standard.cpp:            CHash160().Write(&vSolutions[0][0], vSolutions[0].size()).Finalize(h160);
    7	test/getarg_tests.cpp:    ParseParameters(vecChar.size(), &vecChar[0]);
    8	test/skiplist_tests.cpp:        BOOST_CHECK(vIndex[from].GetAncestor(0) == &vIndex[0]);
    9	wallet/crypter.cpp:    size_t nLen = enc.Encrypt(&vchPlaintext[0], vchPlaintext.size(), &vchCiphertext[0]);
   10	wallet/crypter.cpp:    nLen = dec.Decrypt(&vchCiphertext[0], vchCiphertext.size(), &vchPlaintext[0]);
   11	wallet/rpcwallet.cpp:    return EncodeBase64(&vchSig[0], vchSig.size());
   12	wallet/wallet.cpp:    GetStrongRandBytes(&vMasterKey[0], WALLET_CRYPTO_KEY_SIZE);

(there are a lot more worth checking if you don't use the hungarian notation v prefix as a filter)

[TheBlueMatt]

AFAICT, this is equivalent to sizeof(C) (std::basic_string<C>::value_type is std::basic_string<C, std::char_traits<C>>::value_type which is std::char_traits<C>::char_type, which is C).

[laanwj]

@TheBlueMatt is right

[JeremyRubin]

Yep, I was aware :) I felt that the version I chose was more similar to the existing code (the size of whatever str[0] returns), but if everyone is comfortable with sizeof(C) I can change it.

[JeremyRubin]

👍 fixed

[TheBlueMatt]

Hmm, based on cppreference I cant tell if this is well-defined with vchRet.size() == 0 or not.

[dcousens]

From my understanding (it doesn't seem ambiguous, but, it certainly isn't definitive), a len of 0 is well-defined, and simply an empty string. Aka, equivalent to std::string().
I don't have a definitive statement from the reference.

[laanwj]

Yes, a string with size 0 is well-defined. Calling .data() on a string of size 0 is also defined. This should be ok.

[sipa]

The question is whether calling std::string(garbage_pointer, 0) is well-defined. I can't tell from what cppreference.com says (The behavior is undefined if s does not point at an array of at least count elements of CharT, including the case when s is a null pointer.).

As an alternative, what about std::string(vchRet.begin(), vchRet.end())?

[laanwj]

I read that as: If the count is 0, it always points at least 0 elements, thus "point at an array of at least count elements of CharT" is always satisfied.

Alternatively: if count is 0 it would be wrong to dereference the pointer's address at all.

[sipa]

I'm not convinced that "a pointer to an array of 0 elements of type T" can be any pointer. I think it's extremely unlikely to matter, and that every C++ std library effectively is implemented in a way that makes the constructor, when invoked with (pointer, 0) effectively ignore pointer, however.

[TheBlueMatt]

I mean when cppreference is ambiguous thats probably a good sign that trying to parse a wiki is going to get your an incorrect result...best to go to the standard if in doubt in that case. :/

[JeremyRubin]

I don't think it is can be a garbage pointer. vector.data() is guaranteed to return a valid range (even when empty), which I think would require that it have proper alignment.

[laanwj]

Just to be complete: this is not the same as std::string(vchRet.begin(), vchRet.end()) in the general case. When creating an iterator range, if the type of vch is different from the type of string, a per-element cast will happen. So string.size() will be vch.size(). Usual issues with casting between signed and unsigned apply.

std::string((const char*)vchRet.data(), vchRet.size()) however just return the memory area with all the elements of the vector as a string. So string.size() will be vch.size()*sizeof(vch::value_type). It is likely also more efficient.

[gmaxwell]

Concept ACK.

[practicalswift]

Concept ACK! 👍

[JeremyRubin]

Rebased.

[ryanofsky]

In commit: "Cleanup (safe, it was checked) 0-subscript in MurmurHash3"

Could you drop this (const uint8_t*) cast here? It shouldn't be needed since we already call vDataToHash.data() with no cast above on line 27.

[ryanofsky]

In commit "Fix 0-subscript in utilstrencodings.cpp"

Should put back std:: prefixes.

[ryanofsky]

In commit "Remove uneccessary branches in utilstrencodings string constructors."

Some of the changes in this commit should be squashed into previous commit "In commit "Fix 0-subscript in utilstrencodings.cpp."

Also missing an n in unnecessary.

[ryanofsky]

In commit "Fix 0-subscript in utilstrencodings.cpp"

Bad merge here.

[ryanofsky]

In commit "Fix subscript-0 in streams.h"

It would seem more direct (and shorter) to write sizeof(*vch.data()).

[JeremyRubin]

@ryanofsky feedback addressed.

[ryanofsky]

utACK 275caff. Only changes since previous review (69fd8ab) were addressing feedback and combining / rearranging some of the commits.

[laanwj]

Needs rebase again (sorry).

[JeremyRubin]

@laanwj rebased.

The addrdb commit went away because of cf68a48

[MeshCollider]

Looks good to me, utACK

[ryanofsky]

utACK 30ac768. Changes since previous review were addrdb changes going away and some rearranged commits.

[sipa]

utACK 30ac768

[TheBlueMatt]

utACK 30ac768

[sipa]

utACK 30ac768