CScript += operator double free if destination and operand are the same. #11114

Describe the issue

Can you reliably reproduce the issue?

If so, please list the steps to reproduce below:

CScript cs;
cs << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f")
<< OP_CHECKSIG;
cs += cs;

Expected behaviour

Safe behaviour: no double free.

Actual behaviour

Insecure behaviour: double-free.

Screenshots.

What version of bitcoin-core are you using?

bitcoin-0.14.2, self-compiled

Machine specs:

• OS: Linux
• CPU:
• RAM:
• Disk size:
• Disk Type (HD/SDD):

Any extra information that might be useful in the debugging process.

AddressSanitizer backtrace:

=================================================================
==3429==ERROR: AddressSanitizer: heap-use-after-free on address
0x60b000000040 at pc 0x0000004f919c bp 0x7ffe66520c30 sp
0x7ffe66520c28
READ of size 1 at 0x60b000000040 thread T0
    [#0](/bitcoin-bitcoin/0/) 0x4f919b in void prevector<28u, unsigned char, unsigned int,
int>::insert<prevector<28u, unsigned char, unsigned int,
int>::const_iterator>(prevector<28u, unsigned char, unsigned int,
int>::iterator, prevector<28u, unsigned char, unsigned int,
int>::const_iterator, prevector<28u, unsigned char, unsigned int,
int>::const_iterator)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/prevector.h:378:52
    [#1](/bitcoin-bitcoin/1/) 0x4f919b in CScript::operator+=(CScript const&)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/script/script.h:403
    [#2](/bitcoin-bitcoin/2/) 0x4f919b in main
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/cscript-test.cpp:11
    [#3](/bitcoin-bitcoin/3/) 0x7f3667db3b44 in __libc_start_main
/build/glibc-6V9RKT/glibc-2.19/csu/libc-start.c:287
    [#4](/bitcoin-bitcoin/4/) 0x420d1f in _start
(/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/a.out+0x420d1f)

0x60b000000040 is located 0 bytes inside of 99-byte region
[0x60b000000040,0x60b0000000a3)
freed by thread T0 here:
    [#0](/bitcoin-bitcoin/0/) 0x4c9015 in realloc
(/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/a.out+0x4c9015)
    [#1](/bitcoin-bitcoin/1/) 0x4fe2b9 in prevector<28u, unsigned char, unsigned int,
int>::change_capacity(unsigned int)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/prevector.h:177:54
    [#2](/bitcoin-bitcoin/2/) 0x4f8a54 in void prevector<28u, unsigned char, unsigned int,
int>::insert<prevector<28u, unsigned char, unsigned int,
int>::const_iterator>(prevector<28u, unsigned char, unsigned int,
int>::iterator, prevector<28u, unsigned char, unsigned int,
int>::const_iterator, prevector<28u, unsigned char, unsigned int,
int>::const_iterator)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/prevector.h:373:13
    [#3](/bitcoin-bitcoin/3/) 0x4f8a54 in CScript::operator+=(CScript const&)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/script/script.h:403
    [#4](/bitcoin-bitcoin/4/) 0x4f8a54 in main
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/cscript-test.cpp:11
    [#5](/bitcoin-bitcoin/5/) 0x7f3667db3b44 in __libc_start_main
/build/glibc-6V9RKT/glibc-2.19/csu/libc-start.c:287

previously allocated by thread T0 here:
    [#0](/bitcoin-bitcoin/0/) 0x4c8c73 in __interceptor_malloc
(/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/a.out+0x4c8c73)
    [#1](/bitcoin-bitcoin/1/) 0x4fe0f1 in prevector<28u, unsigned char, unsigned int,
int>::change_capacity(unsigned int)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/prevector.h:181:57
    [#2](/bitcoin-bitcoin/2/) 0x4fb16c in void prevector<28u, unsigned char, unsigned int,
int>::insert<__gnu_cxx::__normal_iterator<unsigned char const*,
std::vector<unsigned char, std::allocator<unsigned char> > >
>(prevector<28u, unsigned char, unsigned int, int>::iterator,
__gnu_cxx::__normal_iterator<unsigned char const*,
std::vector<unsigned char, std::allocator<unsigned char> > >,
__gnu_cxx::__normal_iterator<unsigned char const*,
std::vector<unsigned char, std::allocator<unsigned char> > >)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/prevector.h:373:13
    [#3](/bitcoin-bitcoin/3/) 0x4fb16c in CScript::operator<<(std::vector<unsigned char,
std::allocator<unsigned char> > const&)
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/../src/script/script.h:462
    [#4](/bitcoin-bitcoin/4/) 0x4f8503 in main
/home/jhg/extrahd/bitcoin-fuzzer/bitcoin-0.14.2/fuzz-pr/cscript-test.cpp:10:8
    [#5](/bitcoin-bitcoin/5/) 0x7f3667db3b44 in __libc_start_main
/build/glibc-6V9RKT/glibc-2.19/csu/libc-start.c:287

Reason that this is priority low is that there is no sensible reason to add a script to itself.

It would be good to avoid this potential pitfall, though, either by compile time / runtime assertion to make it impossible or by making it work as expected (and adding it to the tests).

Nice find @guidovranken. Keep on fuzzing!

Out of scope but have you checked other cases where this can happen? There are some T& operator+=(const T&) around and other operators too.

@promag

$ git grep -A5 -E '&.*operator\+=.*&' | grep -3 insert
--
src/script/script.h:    CScript& operator+=(const CScript& b)
src/script/script.h-    {
src/script/script.h-        insert(end(), b.begin(), b.end());
src/script/script.h-        return *this;
src/script/script.h-    }
src/script/script.h-
--
src/streams.h:    CDataStream& operator+=(const CDataStream& b)
src/streams.h-    {
src/streams.h-        vch.insert(vch.end(), b.begin(), b.end());
src/streams.h-        return *this;
src/streams.h-    }
src/streams.h-

Fixed in #11114.