Stop parsing JSON after first finished construct. #6576

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:json-garbage changing 2 files +23 −6

domob1812 commented at 10:50 AM on August 20, 2015: contributor

Fix #6558. In particular, stop parsing JSON after the first object or array is finished. Check that no other garbage follows, and fail the parser if it does.

Stop parsing JSON after first finished construct.

Fix https://github.com/bitcoin/bitcoin/issues/6558.  In particular, stop
parsing JSON after the first object or array is finished.  Check that no
other garbage follows, and fail the parser if it does.

e938122b7b

jonasschnelli commented at 11:26 AM on August 20, 2015: contributor

Thanks!

Tested ACK.
laanwj commented at 1:31 PM on August 20, 2015: member

utACK. Looks good to me. I remember solving a similar problem in json::spirit shortly ago.
laanwj added the label RPC on Aug 20, 2015
domob1812 commented at 3:01 PM on August 20, 2015: contributor

Yeah, I actually noticed this due to an inconsistent behaviour between json_spirit and UniValue. (Note, though, that a different inconsistency remains. While the new behaviour of UniValue is to error out with anything after the first construct, json_spirit just stops parsing and ignores everything that follows.)
laanwj commented at 8:50 AM on August 21, 2015: member

json_spirit just stops parsing and ignores everything that follows

Yes, that's what I fixed. I also changed it to error out with trailing garbage. But I didn't notice that UniValue brought back that behavior because the bug (#6226) didn't return: ParseNonRFCJSONValue wraps the value in an array to be able to have top-level numbers and booleans, so it was already non-tolerant of trailing garbage.
in src/univalue/univalue_read.cpp:None in e938122b7b
```
 376 | @@ -377,9 +377,11 @@ bool UniValue::read(const char *raw)
 377 |          default:
 378 |              return false;
 379 |          }
 380 | -    }
 381 | +    } while (!stack.empty ());
 382 |  
 383 | -    if (stack.size() != 0)
 384 | +    /* Check that nothing follows the initial construct (parsed above).  */
 385 | +    tok = getJsonToken(tokenVal, consumed, raw);
```
laanwj commented at 8:52 AM on August 21, 2015:

Nit: I'd move the definitions of tokenval and consumed here, as they're only used here.

domob1812 commented at 8:55 AM on August 21, 2015:

Well, they are also used in the loop. That's why I moved them above it. Of course, we could also define them both locally in the loop and then here. Would you prefer that?

laanwj commented at 8:56 AM on August 21, 2015:

Not necessary. I missed that, then, sorry.
laanwj merged this on Aug 24, 2015
laanwj closed this on Aug 24, 2015
laanwj referenced this in commit da9beb288d on Aug 24, 2015
domob1812 deleted the branch on Aug 25, 2015
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6558 UniValue should only parse the first JSON construct of a line