Fix OOM when deserializing UTXO entries with invalid length#7933
Merged
laanwj merged 5 commits intobitcoin:masterfrom Apr 26, 2016
Merged
Fix OOM when deserializing UTXO entries with invalid length#7933laanwj merged 5 commits intobitcoin:masterfrom
laanwj merged 5 commits intobitcoin:masterfrom
Conversation
sipa
force-pushed
the
fixcompressoroom
branch
2 times, most recently
from
87f71d9 to
236e132
Compare
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
Author
|
The test (without fix) would use 2 GB+ RAM, and segfault. With fix, it will
work fine. I'll add one.
|
Member
Maybe we can cap it off before that happens? |
Previously disk corruption would cause an assert instead of an exception.
Member
Author
|
Added a test, and included #7936 (the test fails without). |
Member
|
utACK 1e44169 |
Member
|
Can confirm the test fails: |
Contributor
|
utACK 1e44169 |
laanwj
added a commit
that referenced
this pull request
Apr 26, 2016
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
…ializing_utxo, r=daira Upstream: fix out of memory problem when deserializing utxo bitcoin/bitcoin#7933
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Oct 19, 2017
…lid 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)
random-zebra
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
Oct 9, 2019
9f0868a [Tests] Add tests for CCoins deserialization (random-zebra) 5006d45 CDataStream::ignore Throw exception instead of assert on negative nSize (random-zebra) b8bc0d5 [Bug] Fix OOM when deserializing UTXO entries with invalid length (random-zebra) 0657a13 [Script] Treat overly long scriptPubKeys as unspendable (random-zebra) 4bfc161 [Script] Introduce constant for maximum CScript length (random-zebra) Pull request description: ref: bitcoin#7933 ACKs for top commit: furszy: Good, tests passing 👍 , ACK 9f0868a Warrows: ACK 9f0868a Tree-SHA512: 9f978d55cc2564ff905642fe624df43f502a297fbc966480164556328091933a9d6eb861bf287f1d07edb1d0e363d0a63ce76df7f01f01b9e73f69ba87a5576f
akshaynexus
added a commit
to dogecash/dogecash-old
that referenced
this pull request
Oct 26, 2019
9f0868a [Tests] Add tests for CCoins deserialization (random-zebra) 5006d45 CDataStream::ignore Throw exception instead of assert on negative nSize (random-zebra) b8bc0d5 [Bug] Fix OOM when deserializing UTXO entries with invalid length (random-zebra) 0657a13 [Script] Treat overly long scriptPubKeys as unspendable (random-zebra) 4bfc161 [Script] Introduce constant for maximum CScript length (random-zebra) Pull request description: ref: bitcoin/bitcoin#7933 ACKs for top commit: furszy: Good, tests passing 👍 , ACK 9f0868a Warrows: ACK 9f0868a Tree-SHA512: 9f978d55cc2564ff905642fe624df43f502a297fbc966480164556328091933a9d6eb861bf287f1d07edb1d0e363d0a63ce76df7f01f01b9e73f69ba87a5576f
akshaynexus
added a commit
to dogecash/dogecash-old
that referenced
this pull request
Oct 26, 2019
9f0868a [Tests] Add tests for CCoins deserialization (random-zebra) 5006d45 CDataStream::ignore Throw exception instead of assert on negative nSize (random-zebra) b8bc0d5 [Bug] Fix OOM when deserializing UTXO entries with invalid length (random-zebra) 0657a13 [Script] Treat overly long scriptPubKeys as unspendable (random-zebra) 4bfc161 [Script] Introduce constant for maximum CScript length (random-zebra) Pull request description: ref: bitcoin/bitcoin#7933 ACKs for top commit: furszy: Good, tests passing 👍 , ACK 9f0868a Warrows: ACK 9f0868a Tree-SHA512: 9f978d55cc2564ff905642fe624df43f502a297fbc966480164556328091933a9d6eb861bf287f1d07edb1d0e363d0a63ce76df7f01f01b9e73f69ba87a5576f
akshaynexus
added a commit
to dogecash/dogecash-old
that referenced
this pull request
Oct 26, 2019
9f0868a [Tests] Add tests for CCoins deserialization (random-zebra) 5006d45 CDataStream::ignore Throw exception instead of assert on negative nSize (random-zebra) b8bc0d5 [Bug] Fix OOM when deserializing UTXO entries with invalid length (random-zebra) 0657a13 [Script] Treat overly long scriptPubKeys as unspendable (random-zebra) 4bfc161 [Script] Introduce constant for maximum CScript length (random-zebra) Pull request description: ref: bitcoin/bitcoin#7933 ACKs for top commit: furszy: Good, tests passing 👍 , ACK 9f0868a Warrows: ACK 9f0868a Tree-SHA512: 9f978d55cc2564ff905642fe624df43f502a297fbc966480164556328091933a9d6eb861bf287f1d07edb1d0e363d0a63ce76df7f01f01b9e73f69ba87a5576f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.