Stop parsing JSON after first finished construct. #6576

Merged
merged 1 commit into from Aug 24, 2015

Conversation

Projects
None yet
3 participants
@domob1812
Contributor

domob1812 commented Aug 20, 2015

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 #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.
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 20, 2015

Member

Thanks!

Tested ACK.

Member

jonasschnelli commented Aug 20, 2015

Thanks!

Tested ACK.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 20, 2015

Member

utACK. Looks good to me. I remember solving a similar problem in json::spirit shortly ago.

Member

laanwj commented Aug 20, 2015

utACK. Looks good to me. I remember solving a similar problem in json::spirit shortly ago.

@laanwj laanwj added the RPC/REST/ZMQ label Aug 20, 2015

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Aug 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.)

Contributor

domob1812 commented Aug 20, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 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.

Member

laanwj commented Aug 21, 2015

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.

- if (stack.size() != 0)
+ /* Check that nothing follows the initial construct (parsed above). */
+ tok = getJsonToken(tokenVal, consumed, raw);

This comment has been minimized.

@laanwj

laanwj Aug 21, 2015

Member

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

@laanwj

laanwj Aug 21, 2015

Member

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

This comment has been minimized.

@domob1812

domob1812 Aug 21, 2015

Contributor

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?

@domob1812

domob1812 Aug 21, 2015

Contributor

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?

This comment has been minimized.

@laanwj

laanwj Aug 21, 2015

Member

Not necessary. I missed that, then, sorry.

@laanwj

laanwj Aug 21, 2015

Member

Not necessary. I missed that, then, sorry.

@laanwj laanwj merged commit e938122 into bitcoin:master Aug 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 24, 2015

Merge pull request #6576
e938122 Stop parsing JSON after first finished construct. (Daniel Kraft)

@domob1812 domob1812 deleted the domob1812:json-garbage branch Aug 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment