Skip to content
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 univalue handling of \u0000 characters. #6266

Merged
merged 1 commit into from Jun 12, 2015

Conversation

@domob1812
Copy link
Contributor

@domob1812 domob1812 commented Jun 10, 2015

Univalue's parsing of \u escape sequences did not handle NUL characters correctly. They were, effectively, dropped. The extended test-case fails with the old code, and is fixed with this patch.

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Jun 10, 2015
Currently, the rest.py test fails due to a bug in Univalue with NUL
characters.  This will, hopefully, be fixed upstream in Bitcoin soon.
See my patch:

  bitcoin/bitcoin#6266

Conflicts:
	contrib/gitian-descriptors/gitian-linux.yml
	src/init.cpp
	src/rpcrawtransaction.cpp
	src/rpcserver.cpp
	src/rpcserver.h
	src/test/Checkpoints_tests.cpp
	src/wallet/rpcwallet.cpp
@laanwj
Copy link
Member

@laanwj laanwj commented Jun 10, 2015

Thanks for thinking about NUL characters, they're a pet peeve of mine.
utACK

@laanwj laanwj added the RPC/REST/ZMQ label Jun 10, 2015
@jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Jun 10, 2015

Concept ACK - IMO this needs a really close review to ensure you don't overflow e.g. the buf that shrank from 4 to 3

@laanwj
laanwj reviewed Jun 11, 2015
View changes
src/univalue/univalue_read.cpp Outdated
@@ -188,7 +188,7 @@ enum jtokentype getJsonToken(string& tokenVal, unsigned int& consumed,
case 't': valStr += "\t"; break;

case 'u': {
char buf[4] = {0,0,0,0};
char buf[3];

This comment has been minimized.

@laanwj

laanwj Jun 11, 2015
Member

This absolutely needs review, but the changes look very sane to me. The buffer size is decreased from 4 to 3 because the last slot was only there to hold a terminating NUL character. This is no longer necessary because the new code counts characters. If this has an overflow bug, it was already there.

I wonder one thing though: could we push_back the characters into valStr directly instead of the intermediate buf? This would avoid any overflow risk.

This comment has been minimized.

@domob1812

domob1812 Jun 11, 2015
Author Contributor

Exactly that was my rationale. The buffer will only ever be accessed in the iterator range [buf, last), and last itself is incremented at most three times.

However, I like your suggestion of using push_back directly! I think that makes sense and will rewrite the code accordingly.

Univalue's parsing of \u escape sequences did not handle NUL characters
correctly.  They were, effectively, dropped.  The extended test-case
fails with the old code, and is fixed with this patch.
@domob1812 domob1812 force-pushed the domob1812:fix-univalue-nul branch to 0cc7b23 Jun 11, 2015
@domob1812
Copy link
Contributor Author

@domob1812 domob1812 commented Jun 11, 2015

I've changed the patch to directly push the characters onto the result instead of using the temporary buffer. This should be cleaner and less prone to potential overflow bugs.

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 11, 2015

Thanks. re-utACK

@jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Jun 11, 2015

ACK

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Jun 11, 2015

Tested & reviewed ACK

@laanwj laanwj merged commit 0cc7b23 into bitcoin:master Jun 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jun 12, 2015
0cc7b23 Fix univalue handling of \u0000 characters. (Daniel Kraft)
@domob1812 domob1812 deleted the domob1812:fix-univalue-nul branch Jun 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.