Fix invalid memory access in CScript::operator+= (guidovranken, ajtowns) #11284

pull ajtowns wants to merge 1 commits into bitcoin:master from ajtowns:cscript_insert changing 2 files +18 −0

ajtowns commented at 5:30 AM on September 8, 2017: member

This is a fix for #11114 -- invoking "s += s" gets turned into "s.insert(s.end(), s.begin(), s.end())" which can result in an invalid memory access is s.capacity() < 2*s.size() (because s gets resized and possibly moved, so s.begin() and s.end() become invalid references when reading the values to be appended).

The fix is straightforward: reserve enough space in advance, so that insert() doesn't need to resize and thus its arguments remain valid.

A simple test case is added as well; though you probably need to run it via valgrind to actually catch the problem when it's not fixed...
practicalswift commented at 7:28 AM on September 8, 2017: contributor

Concept ACK. Nice!
donaloconnor commented at 8:21 AM on September 8, 2017: contributor

ACK - good spot.
dcousens approved
in src/script/script.h:423 in 84a8b8ab28 outdated
```
 419 | @@ -420,6 +420,7 @@ class CScript : public CScriptBase
 420 |  
 421 |      CScript& operator+=(const CScript& b)
 422 |      {
 423 | +        reserve(size()+b.size());
```
sipa commented at 2:19 AM on September 11, 2017:

Ultranit: spaces around +.

ajtowns commented at 3:43 AM on September 11, 2017:

Ooops. Fixed. Ran the clang-format snippet over it; also changed the declaration of "hex" in the test code.
sipa commented at 2:20 AM on September 11, 2017: member

utACK 84a8b8ab28cf379f1909f0bfc63c9eb25fba4dbd

I was going to suggest just outlawing adding a CScript to itself, but this fix is so small there's no point doing anything else.
Fix invalid memory access in CScript::operator+= d601f16621
ajtowns force-pushed on Sep 11, 2017

ajtowns commented at 3:47 AM on September 11, 2017: member

Testing with the updated script_tests.cpp -- without reserve():

$ valgrind ./test_bitcoin -t script_tests -p
[...]
==26488== Invalid read of size 1
==26488==    at 0x305BE8: insert<prevector<28, unsigned char>::const_iterator> (prevector.h:379)
==26488==    by 0x305BE8: CScript::operator+=(CScript const&) (script.h:423)
==26488==    by 0x2D8BF2: script_tests::script_c_append_self::test_method() (script_tests.cpp:1467)

with reserve():

$ valgrind ./test_bitcoin -t script_tests -p
[...]
Running 12 test cases...

0%   10   20   30   40   50   60   70   80   90   100%
|----|----|----|----|----|----|----|----|----|----|
***************************************************

*** No errors detected
[...]

jnewbery commented at 9:25 PM on September 15, 2017: member

I believe common practice would be to credit the reporter (@guidovranken) in the commit message.
MarcoFalke renamed this:
~~Fix invalid memory access in CScript::operator+=~~
Fix invalid memory access in CScript::operator+= (guidovranken, ajtowns)
on Sep 18, 2017
laanwj commented at 12:46 PM on October 2, 2017: member

utACK d601f16
laanwj merged this on Oct 2, 2017
laanwj closed this on Oct 2, 2017
laanwj referenced this in commit 10bee0dd4f on Oct 2, 2017
PastaPastaPasta referenced this in commit e6cc9da0dc on Jan 17, 2020
PastaPastaPasta referenced this in commit 89831ef791 on Jan 22, 2020
PastaPastaPasta referenced this in commit 134c28bc8e on Jan 22, 2020
PastaPastaPasta referenced this in commit 07296925a8 on Jan 29, 2020
PastaPastaPasta referenced this in commit 18745c3683 on Jan 29, 2020
PastaPastaPasta referenced this in commit eddba9c974 on Jan 31, 2020
ckti referenced this in commit 4d8861a769 on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

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