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

ajtowns · 2017-09-08T05:30:31Z

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 · 2017-09-08T07:28:56Z

Concept ACK. Nice!

donaloconnor · 2017-09-08T08:21:28Z

ACK - good spot.

sipa

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.

sipa · 2017-09-11T02:19:06Z

src/script/script.h

@@ -420,6 +420,7 @@ class CScript : public CScriptBase

    CScript& operator+=(const CScript& b)
    {
+        reserve(size()+b.size());


Ultranit: spaces around +.

Ooops. Fixed. Ran the clang-format snippet over it; also changed the declaration of "hex" in the test code.

ajtowns · 2017-09-11T03:47:23Z

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 · 2017-09-15T21:25:44Z

I believe common practice would be to credit the reporter (@guidovranken) in the commit message.

laanwj · 2017-10-02T12:46:33Z

utACK d601f16

…vranken, ajtowns) d601f16 Fix invalid memory access in CScript::operator+= (Anthony Towns) Pull request description: 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... Tree-SHA512: 4720d0c17463fdc43b344c45fe603423d20b30d48da1b9d85eeedc505d7f34db1ed5495ef1556459ae962a94717e3c6e8fc441763771901efea210d01322b7ef

… (guidovranken, ajtowns) d601f16 Fix invalid memory access in CScript::operator+= (Anthony Towns) Pull request description: This is a fix for bitcoin#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... Tree-SHA512: 4720d0c17463fdc43b344c45fe603423d20b30d48da1b9d85eeedc505d7f34db1ed5495ef1556459ae962a94717e3c6e8fc441763771901efea210d01322b7ef

dcousens approved these changes Sep 11, 2017

View reviewed changes

sipa reviewed Sep 11, 2017

View reviewed changes

Fix invalid memory access in CScript::operator+=

d601f16

ajtowns force-pushed the cscript_insert branch from 84a8b8a to d601f16 Compare September 11, 2017 03:41

maflcko changed the title ~~Fix invalid memory access in CScript::operator+=~~ Fix invalid memory access in CScript::operator+= (guidovranken, ajtowns) Sep 18, 2017

laanwj merged commit d601f16 into bitcoin:master Oct 2, 2017

sickpig mentioned this pull request Jul 17, 2019

Fix invalid memory access in CScript::operator+= BitcoinUnlimited/BitcoinUnlimited#1846

Merged

furszy mentioned this pull request Aug 24, 2021

[Test] BIP62 rule 6 added + script tests updates PIVX-Project/PIVX#2494

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ajtowns commented Sep 8, 2017

practicalswift commented Sep 8, 2017

donaloconnor commented Sep 8, 2017

sipa left a comment

sipa Sep 11, 2017

ajtowns Sep 11, 2017

ajtowns commented Sep 11, 2017

jnewbery commented Sep 15, 2017

laanwj commented Oct 2, 2017

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

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

Conversation

ajtowns commented Sep 8, 2017

practicalswift commented Sep 8, 2017

donaloconnor commented Sep 8, 2017

sipa left a comment

Choose a reason for hiding this comment

sipa Sep 11, 2017

Choose a reason for hiding this comment

ajtowns Sep 11, 2017

Choose a reason for hiding this comment

ajtowns commented Sep 11, 2017

jnewbery commented Sep 15, 2017

laanwj commented Oct 2, 2017