JSON parsing fix from steem PR 2178 #15

pmconrad · 2018-03-01T17:59:42Z

No description provided.

abitmore · 2018-03-01T18:20:12Z

And please add this one: EOSIO/eos@9aa838e

abitmore · 2018-03-01T19:27:33Z

In order to apply the EOS json patch, need to remove variant json::from_stream( buffered_istream& in, parse_type ptype ), otherwise it doesn't compile.

pmconrad · 2018-03-03T09:34:37Z

Picked EOSIO/eos@842e12f

Wrt EOSIO/eos@9aa838e :

This patch cannot be applied, because variant_from_stream is called with different stream types which have inconsistent EOF handling. For example, fc::istream doesn't have an eof(), instead, most implementations/subclasses of fc::istream throw an eof_exception on EOF. fc::stringstream has an eof() method and throws. All streams derived from boost::asio throw and don't have an eof(). fc::ifstream has eof() and a read() method that throws and a read_some() method that does~~n't, thus violating the contract of fc::istream~~. fc::json::from_file calls variant_from_stream with a boost::filesystem::ifstream that only has an eof().

This is a horrible mess. Investigating.

abitmore · 2018-03-04T19:40:00Z

In EOS, before applying the patch, some code got commented out, apparently the code is not being used. Then it compiles.

@pmconrad do you prefer we merge the patch now, or merge after you finished investigation?

pmconrad · 2018-03-04T20:37:21Z

Give me some more time please. Unless you think it's a critical issue.

pmconrad · 2018-03-06T14:26:21Z

Fixed json::from_file, added unit tests. Please review.

It seems the graphene codebase only uses one of the configurable parsers, so there is some superfluous stuff in there.

pmconrad · 2018-03-06T21:07:49Z

Fix is incomplete (json::variants_from_string uses relaxed parser)

abitmore

Mostly fine. Need to fix another issue as discussed in Telegram.

abitmore · 2018-03-06T21:56:42Z

src/io/json.cpp

-          default:
-              FC_ASSERT( false, "Unknown JSON parser type {ptype}", ("ptype", ptype) );
-      }
+      fc::istream_ptr in( new fc::ifstream( p ) );


Need std::ios::binary here.

By the way, need std::ios::binary in from_file(...) below as well.

ifstream always calls open with std::io::binary, see

bitshares-fc/src/io/fstream.cpp

Line 64 in c16bb20

my->ifs.open( bfp, std::ios::binary );

Got it. Thanks. (Just noticed the 2 comments above are talking about same code).

abitmore · 2018-03-06T22:16:13Z

By the way, in void to_stream( T& os, const variant& v, json::output_formatting format ), when handling variant::int64_type, didn't check for negative value.

False alarm.

abitmore · 2018-03-07T16:16:16Z

include/fc/io/json_relaxed.hpp

-              FC_THROW_EXCEPTION( parse_error_exception, "Unexpected char '${c}' in \"${s}\"",
-                                 ("c", c)("s", stringFromToken(in)) );
-         }
+         case '"':


How about single quotes? According to stringFromStream(...), If strict is false, need to support it and raw strings etc.

abitmore · 2018-03-07T16:25:19Z

include/fc/io/json_relaxed.hpp

+         case 0:
+         default:
+            FC_THROW_EXCEPTION( parse_error_exception, "Unexpected char '${c}' in \"${s}\"",
+                                ("c", c)("s", stringFromToken(in)) );


How about '\'? According to other code, unquoted strings are allowed in non-strict mode.

abitmore · 2018-03-07T16:33:07Z

src/io/json.cpp

@@ -188,18 +190,17 @@ namespace fc
               continue;
            }


So ',' can be omitted, e.g. replaced by spaces?
And allow/ignore something like , , ,,,?

Noticed that too. Question is, how much of the original behaviour do we want to change? (Same for other comments above.)

abitmore · 2018-03-07T17:07:39Z

src/io/json.cpp

@@ -276,6 +290,7 @@ namespace fc
                 if (dot)
                    FC_THROW_EXCEPTION(parse_error_exception, "Can't parse a number with two decimal places");
                 dot = true;
+                 [[fallthrough]];


This attribute is since C++17. I heard Visual Studio doesn't support it but will generate an error (I didn't verify).

Oh, I see. Unknown attributes are to be ignored - but only since C++17. :-/

abitmore · 2018-03-07T17:17:28Z

src/io/json.cpp

@@ -299,16 +314,20 @@ namespace fc
        }
      }
      catch (fc::eof_exception&)
-      {
+      { // EOF ends the loop


'\0' will ends the loop as well, but hasn't been handled.
E.G. if the string is "-\0[whatever]", ss.str() would be "-", so it wouldn't get caught by the "obviously wrong things" check below, finally an exception will be thrown on return to_int64(str);.

Yes, but the same is true for "-[whatever]".
The logic here is: a number is a sequence of digits 0-9, possibly separated once by a dot, and possibly prefixed with a single dash. It doesn't matter if the [whatever] sequence starts with a \0 or something else.
Will add "-" to the list of obviously wrong things.

abitmore · 2018-03-07T18:40:03Z

src/io/json.cpp

              if( format == json::stringify_large_ints_and_doubles &&
-                  i > 0xffffffff )
+                  v.as_int64() > 0xffffffff )


Do we need to quote it if v.as_int64() < -(int64_t)0xffffffff?

I think we should.

abitmore · 2018-03-07T19:30:38Z

src/io/json.cpp

@@ -616,27 +590,19 @@ namespace fc
              os << "null";
              return;
         case variant::int64_type:
-         {
-              int64_t i = v.as_int64();


Why change this?

Style, mostly.
IMO these blocks are clumsy, and they're only required so you can declare the "i" variable within the case.

abitmore

Looks good.

abitmore · 2018-03-08T08:21:58Z

src/io/json.cpp

              if( format == json::stringify_large_ints_and_doubles &&
-                  i > 0xffffffff )
+                  ( v.as_int64() > 0xffffffff || v.as_int64() < -int64_t(0xffffffff) ) )


Actually I think the range should be [-0x7fffffff, 0x7fffffff]

AFAIK the reason for this is the number representation in some scripting languages like JavaScript, which is based on IEEE doubles with 52 bits of precision. 2^32-1 should be safe, as well as -(2^32-1).

... unless doing bit-shifting.
https://stackoverflow.com/questions/307179/what-is-javascripts-highest-integer-value-that-a-number-can-go-to-without-losin

The string representation will not enable bit shifting on large integers either.

The string representation will not enable bit shifting on large integers either.

True, but the point is, if it represents as a number, it should enable bit shifting, so 2^31 -1 is the max safe number.

abitmore · 2018-03-08T18:28:53Z

@pmconrad to_stream() has recursion issue.

abitmore · 2018-03-08T23:22:58Z

src/io/json.cpp

   {
+      FC_ASSERT( max_depth > 0, "Too many nested objects!" );


There is an issue related to this, although not necessarily caused by it. For example, among this code:

object o( a_nested_object ); variant a; try { to_stream( nested_object, a ); } FC_CAPTURE_AND_RETHROW ( (o) );

FC_CAPTURE_AND_RETHROW (); will try to jsonify o which is the nested object, so it will throw another exception.

We need to carefully check all catches and make sure they don't throw again.

… string.hpp), added handling of recursion errors

abitmore · 2018-03-11T02:58:50Z

@pmconrad something is broken. Unable to make witness_node. Maybe related to the code handling '$' in format_string?

[ 96%] Building CXX object libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/egenesis_full.cpp.o
/app/bts-dev/build/libraries/egenesis/egenesis_full.cpp:43:38: error: unable to find numeric literal operator ‘operator""$’
 static const char genesis_json_array[941747$][40$+1] =

pmconrad · 2018-03-11T08:58:57Z

Sorry, didn't test that.
IMO the fault lies with libraries/egenesis/egenesis_*.cpp.tmpl - in some places they use placeholders of the form ${x}$ instead of the correct ${x}. The previous version of format_string would skip a $ char if it wasn't followed by a {, which I think is wrong.

JSON parsing fix from steem PR 2178

bbe2a8b

Minor optimization

b12205c

abitmore previously approved these changes Mar 4, 2018

View reviewed changes

pmconrad added 4 commits March 6, 2018 14:43

Added unit tests for stringstream, [io]fstream and buffered_[io]stream

0c75709

Added first json test case

72e96e3

Fixed from_file

52f6810

Deduplicate some code

4d782e8

pmconrad dismissed abitmore’s stale review via 4d782e8 March 6, 2018 14:23

abitmore self-assigned this Mar 6, 2018

abitmore previously requested changes Mar 6, 2018

View reviewed changes

abitmore assigned pmconrad and unassigned abitmore Mar 7, 2018

pmconrad added 9 commits March 7, 2018 15:42

Added json write/validate/read test

8c09ff0

Code cleanups and simplifications

e37d9a5

Code deduplication

43fabf6

Avoid legacy_generator

cb9c61f

Make unused parsers compile-time optional

1ae3cc2

Include variants_from_string in parser test

b4f3947

Fixed relaxed parser wrt "" input

4bb8bf7

Applied variant_from_stream fix from regular to relaxed

d336af8

Added test case for recursion depth 240

a7e0c88

pmconrad added 2 commits March 7, 2018 15:44

Fix for recursion depth limitation

90137d4

Added broken_nul_parser to preserve previous behaviour

2bacd5f

pmconrad assigned abitmore and unassigned pmconrad Mar 7, 2018

abitmore reviewed Mar 7, 2018

View reviewed changes

abitmore removed their assignment Mar 7, 2018

Minor fixes

66ed9fc

abitmore approved these changes Mar 7, 2018

View reviewed changes

abitmore requested changes Mar 8, 2018

View reviewed changes

pmconrad and others added 3 commits March 8, 2018 22:33

Make broken_nul_parser usable

1412df1

Added max_depth parameter to all to_/from_ methods

f9802f6

Stringify numbers >MAXINT or <MININT

1331485

abitmore reviewed Mar 8, 2018

View reviewed changes

pmconrad added 2 commits March 9, 2018 18:51

Added test case for exception in exception handling

d24bee8

Moved format_string from variant.cpp to string.cpp (it is declared in…

527daab

… string.hpp), added handling of recursion errors

pmconrad mentioned this pull request Mar 11, 2018

JSON fix bitshares/bitshares-core#714

Merged

abitmore mentioned this pull request Mar 12, 2018

Fix serialization #20

Merged

abitmore approved these changes Mar 13, 2018

View reviewed changes

abitmore merged commit 67e6036 into bitshares:master Mar 13, 2018

abitmore referenced this pull request in EOSIO/eos May 29, 2018

don't fall into forever loop in zero byty in json #3500

a5f1a20

abitmore mentioned this pull request May 30, 2018

Parsing nesting objects in JSON results in segment fault EOSIO/eos#3539

Closed

JSON parsing fix from steem PR 2178 #15

JSON parsing fix from steem PR 2178 #15

Conversation

pmconrad commented Mar 1, 2018

abitmore commented Mar 1, 2018

abitmore commented Mar 1, 2018

pmconrad commented Mar 3, 2018 • edited Loading

abitmore commented Mar 4, 2018

pmconrad commented Mar 4, 2018

pmconrad commented Mar 6, 2018

pmconrad commented Mar 6, 2018

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Mar 8, 2018

Choose a reason for hiding this comment

abitmore commented Mar 11, 2018

pmconrad commented Mar 11, 2018

pmconrad commented Mar 3, 2018 •

edited

Loading