Fix univalue handling of \u0000 characters. #6266
Conversation
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
|
Thanks for thinking about NUL characters, they're a pet peeve of mine. |
|
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 |
src/univalue/univalue_read.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
fix-univalue-nul
branch
from
0ded8d2 to
0cc7b23
Compare
|
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. |
|
Thanks. re-utACK |
|
ACK |
|
Tested & reviewed ACK |
0cc7b23 Fix univalue handling of \u0000 characters. (Daniel Kraft)
Bitcoin 0.12 RPC PRs 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6266 - bitcoin/bitcoin#6257 - bitcoin/bitcoin#6271 - bitcoin/bitcoin#6158 - bitcoin/bitcoin#6307 - bitcoin/bitcoin#6290 - bitcoin/bitcoin#6262 - bitcoin/bitcoin#6088 - bitcoin/bitcoin#6339 - bitcoin/bitcoin#6299 (partial, remainder in #2099) - bitcoin/bitcoin#6350 - bitcoin/bitcoin#6247 - bitcoin/bitcoin#6362 - bitcoin/bitcoin#5486 - bitcoin/bitcoin#6417 - bitcoin/bitcoin#6398 (partial, remainder was included in #1950) - bitcoin/bitcoin#6444 - bitcoin/bitcoin#6456 (partial, remainder was included in #2082) - bitcoin/bitcoin#6380 - bitcoin/bitcoin#6970 Part of #2074.
4 participants
Univalue's parsing of
\uescape 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.