Page MenuHomePhabricator

Merge #9804: Fixes subscript 0 (&var[0]) where should use (var.data()) instead.
ClosedPublic

Authored by nakihito on Tue, May 7, 03:45.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC24ad36f71b92: Merge #9804: Fixes subscript 0 (&var[0]) where should use (var.data()) instead.
Summary

30ac7688e Fix subscript[0] potential bugs in key.cpp (Jeremy Rubin)
4b1c0f2e2 Remove unnecessary branches in utilstrencodings string constructors. (Jeremy Rubin)
e19db7b5a Fix subscript[0] in utilstrencodings.cpp (Jeremy Rubin)
bc2e7fd98 Fix subscript[0] in streams.h (Jeremy Rubin)
4cac0d1e0 Fix subscript[0] in validation.cpp (Jeremy Rubin)
ac658e55f Fix subscript[0] in torcontrol (Jeremy Rubin)
b6856ebed Fix subscript[0] in netaddress.cpp (Jeremy Rubin)
361d95265 Fix subscript[0] in base58.cpp (Jeremy Rubin)
6896dbf16 Cleanup (safe, it was checked) subscript[0] in MurmurHash3 (and cleanup MurmurHash3 to be more clear). (Jeremy Rubin)
96f2119e6 Fix subscript[0] in compressor.cpp (Jeremy Rubin)
500710bd2 Fix 2 subscript[0] bugs in pubkey.cpp, and eliminate one extra size check (Jeremy Rubin)
e0451e3e2 Fix subscript[0] bug in net.cpp if GetGroup returns a 0-sized vector (Jeremy Rubin)

Tree-SHA512: 5b9103652cf8c615bd8f4f32b3573d291d6b67c39e0308ce00100bc6625f346e8e016b4c999f4f34f5c37ae059490a83c3b513deb21f838af785227d06e02362

Backport of Core PR 9693
https://github.com/bitcoin/bitcoin/pull/9804/

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Tue, May 7, 03:45
Owners added a reviewer: Restricted Owners Package.Tue, May 7, 03:45
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, May 7, 03:45
nakihito planned changes to this revision.Tue, May 7, 03:54
nakihito added inline comments.
src/pubkey.cpp
325 ↗(On Diff #8452)

This part here is supposed to be:

if (!ecdsa_signature_parse_der_lax(secp256k1_context_verify, &sig,
                                       vchSig.data(), vchSig.size())) {

However, this causes a compiler error stating .data() is not a member function. I'm not sure how to rectify this.

nakihito requested review of this revision.Tue, May 7, 19:58
nakihito added inline comments.
src/pubkey.cpp
325 ↗(On Diff #8452)

There does not appear to be an equivalent .data() function in boost::sliced_range (at least that I could find). As such, this will have to stay unchanged.

markblundeberg added inline comments.
src/pubkey.cpp
325 ↗(On Diff #8452)

sliced_range doesn't appear anywhere in Core and is an ABC thing from D1572 :/

Fabien added inline comments.Thu, May 9, 07:39
src/pubkey.cpp
325 ↗(On Diff #8452)

What about &vchSig.front() ?

nakihito updated this revision to Diff 8604.Fri, May 10, 18:06

Previously mentioned issue was resolved with Fabien's suggestion of using .front().

Fabien requested changes to this revision.Mon, May 13, 12:44
Fabien added inline comments.
src/hash.cpp
43 ↗(On Diff #8604)

Note to reviewers:
while checking this against UB I found this interesting article: http://www.drdobbs.com/cpp/why-does-c-allow-arithmetic-on-null-poin/240001022.
Conclusion: this is UB in C but not in C++.

src/streams.h
428 ↗(On Diff #8604)

Braces

This revision now requires changes to proceed.Mon, May 13, 12:44
markblundeberg added inline comments.Mon, May 13, 13:52
src/hash.cpp
43 ↗(On Diff #8604)

That is interesting stuff! TIL

nakihito updated this revision to Diff 8633.Mon, May 13, 17:11

Added {}.

Fabien accepted this revision.Mon, May 13, 17:20
This revision is now accepted and ready to land.Mon, May 13, 17:20