Sanitize some wallet serialization #12658
Merged
Conversation
This file contains 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
sipa
changed the title
eklitzke
reviewed
Mar 10, 2018
| std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev | ||
| s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent; | ||
|
|
||
| strFromAccount = std::move(mapValue["fromaccount"]); |
There was a problem hiding this comment.
nit: Instead of move here and erase a few lines below, you can do the search one and localize the logic:
auto it = mapValue.find("fromaccount");
assert(it != mapValue.end()); // optional, if you feel like it
strFromAccount = std::move(it->second);
mapValue.erase(it);One day in the glorious future when we're all using C++17 you can use .extract().
There was a problem hiding this comment.
As this code isn't particularly performance critical, I prefer the current shorter code.
src/wallet/wallet.h
Outdated
| WriteOrderPos(nOrderPos, mapValueCopy); | ||
|
|
||
| std::string strCommentCopy = strComment; | ||
| if (!(mapValueCopy.empty() && _ssExtra.empty())) { |
There was a problem hiding this comment.
nit: consider making this:
if (!mapValueCopy.empty() || !_ssExtra.empty()) {
sipa
force-pushed
the
201803_crazywalletser
branch
from
6ed3ab8
to
42343c7
Compare
|
utACK 42343c7 LGTM. |
ryanofsky
reviewed
Mar 13, 2018
| //! Note: strAccount is serialized as part of the key, not here. | ||
| READWRITE(nCreditDebit); | ||
| READWRITE(nTime); | ||
| READWRITE(LIMITED_STRING(strOtherAccount, 65536)); |
There was a problem hiding this comment.
Not being changed by this PR, but seems bad to me that length is not checked on serialization (only on deserialization).
There was a problem hiding this comment.
Although you could add a sanity check, serialization is probably too late to check it. Ideally anything inputting account/label names should check this to be able to give an useful error and reject setting it before putting it in a data structure.
But yes, out of scope here.
|
utACK 42343c7 |
laanwj
added a commit
that referenced
this pull request
Mar 13, 2018
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from #10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 9, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 9, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 10, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 11, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 11, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 11, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 12, 2020
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#10785, fixing a few of the craziest constness violations in the serialization code. `CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again. Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This is a small subset of changes taken from #10785, fixing a few of the craziest constness violations in the serialization code.
CWalletTxcurrently serializes some of its fields by embedding them in a key-valuemapValue, which is modified (and then fixed up) even from theSerializemethod (for whichmapValueis const).CAccountingEntrygoes even further in that it stores such a map by appending it intostrCommentafter a null char, which is again later fixed up again.Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of
mapValue/strComment.