New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix OOM when deserializing UTXO entries with invalid length #7933

Merged
merged 5 commits into from Apr 26, 2016

Conversation

Projects
None yet
5 participants
@sipa
Member

sipa commented Apr 24, 2016

Thanks to @pstratem for finding this.

The normal vector deserializer reads data in chunks of at most 5 MB, preventing OOM when insane vector lengths are encoded. This protection is not present in CScriptCompressor's specialized deserializer, however, resulting in a potential OOM when very large length descriptors exist, as the target CScript is resized before attempting to read that much data.

However, CScripts have a maximum length above which they're always invalid. We can treat scriptPubKeys with such lengths as unspendable, preventing them from going into the UTXO set even, and skipping them when deserializing.

Note that none of this is exposed to the network, as the P2P code uses normal (pre)vectors, which do have this OOM protection directly in serialize.h.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 25, 2016

Member

I think this needs a test (that fails without this, and succeeds with it). Or are we talking about 'insane lengths' of such magnitude that would result in a very long running test case?

Member

laanwj commented Apr 25, 2016

I think this needs a test (that fails without this, and succeeds with it). Or are we talking about 'insane lengths' of such magnitude that would result in a very long running test case?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 25, 2016

Member
Member

sipa commented Apr 25, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 25, 2016

Member

The test (without fix) would use 2 GB+ RAM, and segfault.

Maybe we can cap it off before that happens?
In any case a test that OOMs and crashes without this, but runs quickly with it, would be great too, it effectively prevents regression.

Member

laanwj commented Apr 25, 2016

The test (without fix) would use 2 GB+ RAM, and segfault.

Maybe we can cap it off before that happens?
In any case a test that OOMs and crashes without this, but runs quickly with it, would be great too, it effectively prevents regression.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 25, 2016

Member

Added a test, and included #7936 (the test fails without).

Member

sipa commented Apr 25, 2016

Added a test, and included #7936 (the test fails without).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 25, 2016

Member

utACK 1e44169

Member

laanwj commented Apr 25, 2016

utACK 1e44169

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 25, 2016

Member

Can confirm the test fails:

$ src/test/test_bitcoin -t coins_tests
Running 3 test cases...
unknown location(0): fatal error in "ccoins_serialization": memory access violation at address: 0x...: no mapping at fault address
test/coins_tests.cpp(411): last checkpoint

*** 1 failure detected in test suite "Bitcoin Test Suite"
Member

MarcoFalke commented Apr 25, 2016

Can confirm the test fails:

$ src/test/test_bitcoin -t coins_tests
Running 3 test cases...
unknown location(0): fatal error in "ccoins_serialization": memory access violation at address: 0x...: no mapping at fault address
test/coins_tests.cpp(411): last checkpoint

*** 1 failure detected in test suite "Bitcoin Test Suite"
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 26, 2016

Contributor

utACK 1e44169

Contributor

dcousens commented Apr 26, 2016

utACK 1e44169

@laanwj laanwj merged commit 1e44169 into bitcoin:master Apr 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 26, 2016

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)

zkbot pushed a commit to zcash/zcash that referenced this pull request Oct 22, 2016

zkbot
Auto merge of #1591 - bitcartel:upstream_7933_fix_out_of_memory_deser…
…ializing_utxo, r=daira

Upstream: fix out of memory problem when deserializing utxo

bitcoin/bitcoin#7933

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Sep 27, 2017

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Oct 12, 2017

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Oct 19, 2017

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Nov 8, 2017

Merge #7933: Fix OOM when deserializing UTXO entries with invalid length
1e44169 Add tests for CCoins deserialization (Pieter Wuille)
5d0434d Fix OOM bug: UTXO entries with invalid script length (Pieter Wuille)
4bf631e CDataStream::ignore Throw exception instead of assert on negative nSize. (Patrick Strateman)
4f87af6 Treat overly long scriptPubKeys as unspendable (Pieter Wuille)
f8e6fb1 Introduce constant for maximum CScript length (Pieter Wuille)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment