Run “Parsing JSON is a Minefield” against univalue #9028

1. 
   laanwj commented at 6:38 am on October 27, 2016: member

   Nicolas Seriot recently published an article comparing various JSON parsers. Some of the problems he found are quite worrying, such as parser crash bugs in common libraries.

   We did a lot of fuzzing so I’d hope we find no or few crash bugs. However it would be interesting to run his test suite (raw test data is available) against univalue to see how univalue compares or whether it finds any crash-level issues or just pedantic ones.

2. laanwj added the label Tests on Oct 27, 2016
3. laanwj added the label Easy to implement on Oct 27, 2016
4. 
   ryanofsky commented at 8:52 pm on November 1, 2016: member

   I looked into this, creating a test driver for UniValue that works with the test suite, and fixing a few issues that this uncovered. There were no test cases that caused crashes, but there was:

   • A test case that showed UniValue improperly accepting a non-minimal utf8 sequence (C0 AF)
   • A test case that showed UniValue returning a partial parsing of a json input with an embedded nul byte ('\0')
   • Some test cases that failed because of missing features in UniValue: utf16 decoding, support for reading plain number and string values not embedded in objects or arrays.

   I created a PR at https://github.com/ryanofsky/bitcoin/pull/1 with a few commits fixing the various issues. Not sending the PR upstream for now, because it’s not clear which of the changes might be desirable to keep permanently.

5. 
   laanwj commented at 2:43 pm on November 2, 2016: member

   The second commit extends UniValue::Read() to support reading standalone number/string/bool/null values that aren’t arrays or objects. It fixes the following tests:

   Slighly off-topic: I like this one. We actually have workarounds in place to do “lonely” JSON value parsing: ParseNonRFCJSONValue in https://github.com/bitcoin/bitcoin/blob/master/src/rpc/client.h#L12 . Would be great if upstream just supported it.

6. 
   ryanofsky commented at 9:05 pm on November 4, 2016: member

   Created upstream PRs at:

   https://github.com/jgarzik/univalue/pull/30 https://github.com/jgarzik/univalue/pull/31 https://github.com/jgarzik/univalue/pull/32 https://github.com/jgarzik/univalue/pull/33

7. 
   ryanofsky commented at 8:13 pm on December 14, 2017: member

   Could close this issue, the fixes are upstream.
8. 
   MarcoFalke commented at 8:19 pm on December 14, 2017: member

   Pending merge is https://github.com/jgarzik/univalue/pull/33, but indeed no need to keep this open with the label ‘good first issue’
9. MarcoFalke closed this on Dec 14, 2017

   ---

10. MarcoFalke removed the label good first issue on Dec 14, 2017
11. MarcoFalke locked this on Sep 8, 2021

Contributors
laanwj ryanofsky MarcoFalke

Labels
Tests