From 37f78ed324a0a081d7765b0e7abad0d8c15d7d91 Mon Sep 17 00:00:00 2001 From: theoreticalbts Date: Fri, 23 Dec 2016 12:36:14 -0500 Subject: [PATCH 1/6] Implement tests for some illegal asset strings #102 --- libraries/protocol/asset.cpp | 8 ++++++-- tests/tests/serialization_tests.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/libraries/protocol/asset.cpp b/libraries/protocol/asset.cpp index 5d6492823c..c9e00b82aa 100644 --- a/libraries/protocol/asset.cpp +++ b/libraries/protocol/asset.cpp @@ -68,9 +68,13 @@ namespace steemit { namespace protocol { result.amount.value -= result.precision(); } auto symbol = s.substr( space_pos + 1 ); + size_t symbol_size = symbol.size(); - if( symbol.size() ) - memcpy( sy+1, symbol.c_str(), std::min(symbol.size(),size_t(6)) ); + if( symbol_size > 0 ) + { + FC_ASSERT( symbol_size <= 6 ); + memcpy( sy+1, symbol.c_str(), symbol_size ); + } return result; } diff --git a/tests/tests/serialization_tests.cpp b/tests/tests/serialization_tests.cpp index 8c9ef58a7e..bd15217086 100644 --- a/tests/tests/serialization_tests.cpp +++ b/tests/tests/serialization_tests.cpp @@ -141,6 +141,35 @@ BOOST_AUTO_TEST_CASE( asset_test ) BOOST_CHECK_EQUAL( sbd.symbol, SBD_SYMBOL); BOOST_CHECK_EQUAL( asset(50, SBD_SYMBOL).to_string(), "0.050 TBD" ); BOOST_CHECK_EQUAL( asset(50000, SBD_SYMBOL).to_string(), "50.000 TBD" ); + + BOOST_CHECK_THROW( steem.set_decimals(100), fc::exception ); + char* steem_sy = (char*) &steem.symbol; + steem_sy[0] = 100; + BOOST_CHECK_THROW( steem.decimals(), fc::exception ); + steem_sy[6] = 'A'; + steem_sy[7] = 'A'; + + auto check_sym = []( const asset& a ) -> std::string + { + auto symbol = a.symbol_name(); + wlog( "symbol_name is ${s}", ("s", symbol) ); + return symbol; + }; + + BOOST_CHECK_THROW( check_sym(steem), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1.00000000000000000000 TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1.000TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1. 333 TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1 .333 TESTS" ), fc::exception ); + asset unusual = asset::from_string( "1. 333 X" ); + FC_ASSERT( unusual.decimals() == 0 ); + FC_ASSERT( unusual.symbol_name() == "333 X" ); + BOOST_CHECK_THROW( asset::from_string( "1 .333 X" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1 .333" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1 1.1" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "11111111111111111111111111111111111111111111111 TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1.1.1 TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1.abc TESTS" ), fc::exception ); } FC_LOG_AND_RETHROW() } From d807d13e5b908dca9f77e972ef3aad8d5c3baab7 Mon Sep 17 00:00:00 2001 From: theoreticalbts Date: Fri, 23 Dec 2016 11:02:53 -0500 Subject: [PATCH 2/6] Fix handling of illegal values in asset strings #102 --- libraries/protocol/asset.cpp | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/libraries/protocol/asset.cpp b/libraries/protocol/asset.cpp index c9e00b82aa..ab20cc581c 100644 --- a/libraries/protocol/asset.cpp +++ b/libraries/protocol/asset.cpp @@ -16,12 +16,12 @@ namespace steemit { namespace protocol { std::string asset::symbol_name()const { auto a = (const char*)&symbol; - assert( a[7] == 0 ); + FC_ASSERT( a[7] == 0 ); return &a[1]; } - int64_t asset::precision()const { - + int64_t asset::precision()const + { static int64_t table[] = { 1, 10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000ll, @@ -29,15 +29,21 @@ namespace steemit { namespace protocol { 100000000000ll, 1000000000000ll, 10000000000000ll, 100000000000000ll }; - return table[ decimals() ]; + uint8_t d = decimals(); + FC_ASSERT( d < 15 ); + return table[ d ]; } - string asset::to_string()const { - string result = fc::to_string(amount.value / precision()); + string asset::to_string()const + { + int64_t prec = precision(); + string result = fc::to_string(amount.value / prec); if( decimals() ) { - auto fract = amount.value % precision(); - result += "." + fc::to_string(precision() + fract).erase(0,1); + auto fract = amount.value % prec; + result += "." + fc::to_string(prec + fract).erase(0,1); + wlog( "prec is ${prec} fract is ${fract} p+f is ${pf} result is ${result}", + ("prec", prec)("fract", fract)("result", result)("pf", prec+fract) ); } return result + " " + symbol_name(); } @@ -50,16 +56,19 @@ namespace steemit { namespace protocol { auto space_pos = s.find( " " ); auto dot_pos = s.find( "." ); + FC_ASSERT( space_pos != std::string::npos ); + asset result; result.symbol = uint64_t(0); auto sy = (char*)&result.symbol; - *sy = (char) dot_pos; // Mask due to undefined architecture behavior auto intpart = s.substr( 0, dot_pos ); result.amount = fc::to_int64(intpart); std::string fractpart; if( dot_pos != std::string::npos ) { + FC_ASSERT( space_pos > dot_pos ); + auto fractpart = "1" + s.substr( dot_pos + 1, space_pos - dot_pos - 1 ); result.set_decimals( fractpart.size() - 1 ); @@ -67,6 +76,10 @@ namespace steemit { namespace protocol { result.amount.value += fc::to_int64(fractpart); result.amount.value -= result.precision(); } + else + { + result.set_decimals( 0 ); + } auto symbol = s.substr( space_pos + 1 ); size_t symbol_size = symbol.size(); From cf93aaa935655ae42316f1954ee855dd139c3683 Mon Sep 17 00:00:00 2001 From: theoreticalbts Date: Fri, 23 Dec 2016 12:23:06 -0500 Subject: [PATCH 3/6] Some more defensive FC_ASSERT() #102 --- libraries/protocol/asset.cpp | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/libraries/protocol/asset.cpp b/libraries/protocol/asset.cpp index ab20cc581c..a4a20eff49 100644 --- a/libraries/protocol/asset.cpp +++ b/libraries/protocol/asset.cpp @@ -2,26 +2,43 @@ #include #include +/* + +The bounds on asset serialization are as follows: + +index : field +0 : decimals +1..6 : symbol + 7 : \0 +*/ + namespace steemit { namespace protocol { typedef boost::multiprecision::int128_t int128_t; - uint8_t asset::decimals()const { + uint8_t asset::decimals()const + { auto a = (const char*)&symbol; - return a[0]; + uint8_t result = uint8_t( a[0] ); + FC_ASSERT( result < 15 ); + return result; } - void asset::set_decimals(uint8_t d){ + + void asset::set_decimals(uint8_t d) + { + FC_ASSERT( d < 15 ); auto a = (char*)&symbol; a[0] = d; } - std::string asset::symbol_name()const { + std::string asset::symbol_name()const + { auto a = (const char*)&symbol; FC_ASSERT( a[7] == 0 ); return &a[1]; } - int64_t asset::precision()const - { + int64_t asset::precision()const + { static int64_t table[] = { 1, 10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000ll, @@ -30,7 +47,6 @@ namespace steemit { namespace protocol { 10000000000000ll, 100000000000000ll }; uint8_t d = decimals(); - FC_ASSERT( d < 15 ); return table[ d ]; } @@ -41,9 +57,11 @@ namespace steemit { namespace protocol { if( decimals() ) { auto fract = amount.value % prec; + // prec is a power of ten, so for example when working with + // 7.005 we have fract = 5, prec = 1000. So prec+fract=1005 + // has the correct number of zeros and we can simply trim the + // leading 1. result += "." + fc::to_string(prec + fract).erase(0,1); - wlog( "prec is ${prec} fract is ${fract} p+f is ${pf} result is ${result}", - ("prec", prec)("fract", fract)("result", result)("pf", prec+fract) ); } return result + " " + symbol_name(); } From b03fdf903e061bbb3dcd6f280ceafd4b6148a23a Mon Sep 17 00:00:00 2001 From: theoreticalbts Date: Fri, 23 Dec 2016 14:13:36 -0500 Subject: [PATCH 4/6] Use more efficient check for non-decimal assets #102 --- libraries/protocol/asset.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/protocol/asset.cpp b/libraries/protocol/asset.cpp index a4a20eff49..d03325e6d7 100644 --- a/libraries/protocol/asset.cpp +++ b/libraries/protocol/asset.cpp @@ -54,7 +54,7 @@ namespace steemit { namespace protocol { { int64_t prec = precision(); string result = fc::to_string(amount.value / prec); - if( decimals() ) + if( prec > 1 ) { auto fract = amount.value % prec; // prec is a power of ten, so for example when working with From f595b8fbd169bb87f8f83ddc8de7a528c0452750 Mon Sep 17 00:00:00 2001 From: Michael Vandeberg Date: Tue, 3 Jan 2017 15:16:25 -0500 Subject: [PATCH 5/6] Fix failure when parsing a string with precision 0. Added more tests for corner cases #102 --- libraries/protocol/asset.cpp | 11 ++++++----- tests/tests/serialization_tests.cpp | 11 +++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/libraries/protocol/asset.cpp b/libraries/protocol/asset.cpp index d03325e6d7..670febd020 100644 --- a/libraries/protocol/asset.cpp +++ b/libraries/protocol/asset.cpp @@ -80,22 +80,23 @@ namespace steemit { namespace protocol { result.symbol = uint64_t(0); auto sy = (char*)&result.symbol; - auto intpart = s.substr( 0, dot_pos ); - result.amount = fc::to_int64(intpart); - std::string fractpart; if( dot_pos != std::string::npos ) { FC_ASSERT( space_pos > dot_pos ); + auto intpart = s.substr( 0, dot_pos ); auto fractpart = "1" + s.substr( dot_pos + 1, space_pos - dot_pos - 1 ); result.set_decimals( fractpart.size() - 1 ); + result.amount = fc::to_int64( intpart ); result.amount.value *= result.precision(); - result.amount.value += fc::to_int64(fractpart); + result.amount.value += fc::to_int64( fractpart ); result.amount.value -= result.precision(); } else { + auto intpart = s.substr( 0, space_pos ); + result.amount = fc::to_int64( intpart ); result.set_decimals( 0 ); } auto symbol = s.substr( space_pos + 1 ); @@ -109,7 +110,7 @@ namespace steemit { namespace protocol { return result; } - FC_CAPTURE_LOG_AND_RETHROW( (from) ) + FC_CAPTURE_AND_RETHROW( (from) ) } bool operator == ( const price& a, const price& b ) diff --git a/tests/tests/serialization_tests.cpp b/tests/tests/serialization_tests.cpp index bd15217086..6e3390590d 100644 --- a/tests/tests/serialization_tests.cpp +++ b/tests/tests/serialization_tests.cpp @@ -127,6 +127,7 @@ BOOST_AUTO_TEST_CASE( asset_test ) BOOST_CHECK_EQUAL( tmp.amount.value, 56 ); BOOST_CHECK( std::abs( steem.to_real() - 123.456 ) < 0.0005 ); + BOOST_CHECK_EQUAL( steem.amount.value, 123456 ); BOOST_CHECK_EQUAL( steem.decimals(), 3 ); BOOST_CHECK_EQUAL( steem.symbol_name(), "TESTS" ); BOOST_CHECK_EQUAL( steem.to_string(), "123.456 TESTS" ); @@ -135,6 +136,7 @@ BOOST_AUTO_TEST_CASE( asset_test ) BOOST_CHECK_EQUAL( asset(50000, STEEM_SYMBOL).to_string(), "50.000 TESTS" ); BOOST_CHECK( std::abs( sbd.to_real() - 654.321 ) < 0.0005 ); + BOOST_CHECK_EQUAL( sbd.amount.value, 654321 ); BOOST_CHECK_EQUAL( sbd.decimals(), 3 ); BOOST_CHECK_EQUAL( sbd.symbol_name(), "TBD" ); BOOST_CHECK_EQUAL( sbd.to_string(), "654.321 TBD" ); @@ -170,6 +172,15 @@ BOOST_AUTO_TEST_CASE( asset_test ) BOOST_CHECK_THROW( asset::from_string( "11111111111111111111111111111111111111111111111 TESTS" ), fc::exception ); BOOST_CHECK_THROW( asset::from_string( "1.1.1 TESTS" ), fc::exception ); BOOST_CHECK_THROW( asset::from_string( "1.abc TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( " TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1.333" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1.333 " ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( " " ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( " " ), fc::exception ); + + BOOST_CHECK_EQUAL( asset::from_string( "100 TESTS" ).amount.value, 100 ); } FC_LOG_AND_RETHROW() } From ba8e560800a5ccb9e6f3b300d56e28f66c0c8f8f Mon Sep 17 00:00:00 2001 From: Michael Vandeberg Date: Tue, 3 Jan 2017 15:20:15 -0500 Subject: [PATCH 6/6] Add documentation for confusing test case #102 --- tests/tests/serialization_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tests/serialization_tests.cpp b/tests/tests/serialization_tests.cpp index 6e3390590d..1523e15912 100644 --- a/tests/tests/serialization_tests.cpp +++ b/tests/tests/serialization_tests.cpp @@ -161,9 +161,9 @@ BOOST_AUTO_TEST_CASE( asset_test ) BOOST_CHECK_THROW( check_sym(steem), fc::exception ); BOOST_CHECK_THROW( asset::from_string( "1.00000000000000000000 TESTS" ), fc::exception ); BOOST_CHECK_THROW( asset::from_string( "1.000TESTS" ), fc::exception ); - BOOST_CHECK_THROW( asset::from_string( "1. 333 TESTS" ), fc::exception ); + BOOST_CHECK_THROW( asset::from_string( "1. 333 TESTS" ), fc::exception ); // Fails because symbol is '333 TESTS', which is too long BOOST_CHECK_THROW( asset::from_string( "1 .333 TESTS" ), fc::exception ); - asset unusual = asset::from_string( "1. 333 X" ); + asset unusual = asset::from_string( "1. 333 X" ); // Passes because symbol '333 X' is short enough FC_ASSERT( unusual.decimals() == 0 ); FC_ASSERT( unusual.symbol_name() == "333 X" ); BOOST_CHECK_THROW( asset::from_string( "1 .333 X" ), fc::exception );