Fix univalue handling of \u0000 characters. #6266

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:fix-univalue-nul changing 2 files +10 −11

domob1812 commented at 1:00 PM on June 10, 2015: contributor

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 referenced this in commit 944e927d86 on Jun 10, 2015
laanwj commented at 4:11 PM on June 10, 2015: member

Thanks for thinking about NUL characters, they're a pet peeve of mine. utACK
laanwj added the label RPC on Jun 10, 2015
jgarzik commented at 6:50 PM on June 10, 2015: contributor

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

Fix univalue handling of \u0000 characters.

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.

0cc7b2352e

in src/univalue/univalue_read.cpp:None in 0ded8d2ed8 outdated
```
 187 | @@ -188,7 +188,7 @@ enum jtokentype getJsonToken(string& tokenVal, unsigned int& consumed,
 188 |                  case 't':  valStr += "\t"; break;
 189 |  
 190 |                  case 'u': {
 191 | -                    char buf[4] = {0,0,0,0};
 192 | +                    char buf[3];
```
laanwj commented at 8:35 AM on June 11, 2015:

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.

domob1812 commented at 9:24 AM on June 11, 2015:

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.
domob1812 force-pushed on Jun 11, 2015
domob1812 commented at 10:10 AM on June 11, 2015: contributor

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 commented at 4:18 PM on June 11, 2015: member

Thanks. re-utACK
jgarzik commented at 4:27 PM on June 11, 2015: contributor

ACK
jonasschnelli commented at 7:49 PM on June 11, 2015: contributor

Tested & reviewed ACK
laanwj merged this on Jun 12, 2015
laanwj closed this on Jun 12, 2015
laanwj referenced this in commit ebab5d3c59 on Jun 12, 2015
domob1812 deleted the branch on Jun 12, 2015
zkbot referenced this in commit 9af55822fb on Feb 15, 2017
zkbot referenced this in commit a7cf698873 on Mar 4, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels