Rational number limit #3793

erak · 2018-03-28T14:11:38Z

axic · 2018-03-28T15:42:04Z

libsolidity/ast/Types.cpp

 			bigint denominator = pow(m_value.denominator(), exponent);
+
+			// Limit size to 4096 bits
+			if (numerator >= pow(bigint(2), rationalMaxBits) - 1)


This pow should be evaluated in the const variable. Also you can use shift instead of pow for power-of-2.

I don't think it is enough to check for the value after it has ben evaluated. It should be guessed before evaluating with pow.

One of the main problems here is the DoS vector (which is just annoying for a user, for example running the compiler in the browser).

axic · 2018-03-28T15:44:04Z

libsolidity/ast/Types.cpp

 			bigint denominator = pow(m_value.denominator(), exponent);
+
+			// Limit size to 4096 bits
+			if (numerator >= pow(bigint(2), rationalMaxBits) - 1)


I don't think it is enough to check for the value after it has ben evaluated. It should be guessed before evaluating with pow.

One of the main problems here is the DoS vector (which is just annoying for a user, for example running the compiler in the browser).

erak · 2018-03-28T17:58:24Z

@axic I've started testing with two examples of those DoS cases and they're not hanging anymore. After the limit is reached, the function is still called, but m_value.denominator() is constant from there on and calling pow() results in the same large number.
Currently the error messages don't help to understand the issue I guess, but there might be some options for making them richer.

chriseth · 2018-03-29T07:47:56Z

libsolidity/ast/Types.cpp

+namespace
+{
+
+uint32_t static const exponentLimit = 9999;


We don't really use uint32_t explicitly in the rest of the codebase. I think I almost exclusively use size_t.

size_t or unsigned is most common in the codebase

chriseth · 2018-03-29T07:48:23Z

libsolidity/ast/Types.cpp


 TypePointer IntegerType::binaryOperatorResult(Token::Value _operator, TypePointer const& _other) const
-{
+{	


Surplus whitespace.

chriseth · 2018-03-29T07:55:05Z

libsolidity/ast/Types.cpp

+			// parse the exponent and check limits
 			bigint exp = bigint(string(expPoint + 1, _literal.value().end()));

 			if (exp > numeric_limits<int32_t>::max() || exp < numeric_limits<int32_t>::min())


This can now be removed.

chriseth · 2018-03-29T07:56:49Z

libsolidity/ast/Types.cpp

 			if (exp > numeric_limits<int32_t>::max() || exp < numeric_limits<int32_t>::min())
 				return make_tuple(false, rational(0));

+			if (abs(exp) >= exponentLimit)


Perhaps it would be better to re-use the same helper in all functions that compute exp for rational numbers. Just pass the base and the exponent, and it will determine if the resulting number still fits our precision.

Also, if we know the base, we can better estimate the size of the resulting number.

chriseth · 2018-03-29T07:58:19Z

Please also add a test for bit-shifts.

axic · 2018-04-04T15:31:09Z

After the limit is reached, the function is still called, but m_value.denominator() is constant from there on and calling pow() results in the same large number.

For those test cases it probably is the case, however for the case of doing a very large exp, it will just "hang" or take a lot of time.

Please add such a test case too.

axic · 2018-04-04T16:43:10Z

Maybe useful to look at the test cases of #3348 (an earlier attempt of the same problem).

Additionally, some random notes:

uint a = 1E1000000 ** 1E1000000;
100000000000E1250 = 1E1261

ekpyron · 2018-04-09T16:27:08Z

test/libsolidity/syntaxTests/types/rational_number_literal_limit.sol

+        uint a;
+        uint b;
+        a = 1e9999;
+				b = 13456232453246423445233516816351681345623245324642344523351681635168.7161354161363516163161354168154E1200;


Indentation

ekpyron · 2018-04-09T17:03:08Z

libsolidity/ast/Types.cpp

+	if(_base > baseMax)
+		return false;
+
+	bigfloat bitsNeeded = bigfloat(_exp) * ceil(log10(bigfloat(_base + 1)));


Maybe log2 instead of log10?

That was a defect ;) log10 is what I wanted but this formula calculated the number of digits and not the number of bits. I've added another formula which estimates the minimum number of bits needed.

axic · 2018-04-09T17:10:28Z

libsolidity/ast/Types.cpp

+{
+	using boost::multiprecision::log10;
+	using boost::multiprecision::ceil;
+	using bigfloat = boost::multiprecision::number<boost::multiprecision::cpp_dec_float<50>>;


Does that mean it allows up to 50 decimal digits? What happens if _base + 1 is larger than 50 decimal digits?

According to boost these are also defined:

typedef number<cpp_dec_float<50> > cpp_dec_float_50; typedef number<cpp_dec_float<100> > cpp_dec_float_100;

@axic The precision will be reduced after creating cpp_dec_float_50 with base + 1. That means a if base has more than 50 digits, any additional digit will be represented with a zero. This does not affect the size of it.

Can you add a test for this? Boost had some bugs where an overflow (with shifts) caused the entire value to be 0.

ekpyron · 2018-04-11T09:13:32Z

libsolidity/ast/Types.cpp

+		return false;
+
+	bigfloat digitsNeeded = bigfloat(_exp) * ceil(log10(bigfloat(_base + 1)));
+	bigfloat bitsNeeded = ceil((digitsNeeded - 1) * log2(bigfloat(10)));


Why calculate digitsNeeded as an intermediate result?
You might as well just use log2 directly to get bitsNeeded instead of multiplying with log2(10) later...

log2(10) * log10(a) = log2(a)

I don't think using 10 as base has any advantage for choosing the correct way of rounding things...

What you want to have in the end is: 2 ^ 4096 - 1 >= _base^_exp or, since it's integers:
2^4096 > _base^_exp.

Hence you want log2(2^4096) > log2(_base^_exp).
Hence 4096 > _exp * log2(_base).

You only need to compensate for potential rounding error in log2(_base).

Safest thing would probably be bitsNeeded = _exp * (floor(log2(_base)) + 1).

Although you may want to treat _base <= 1 separately :-).

Also: do you only use fitsPrecision for _base >= 0? You'd probably need to treat negative _base differently...
And probably also add tests for that.

axic

Also needs a test for a larger base with a smaller exponent, which still exceeds the limit.

chriseth · 2018-04-13T12:26:46Z

libsolidity/ast/Types.cpp

+
+	size_t const bitsMax = 4096;
+	bigint const baseMax = (bigint(2) << bitsMax) - 1;
+	if(_base > baseMax)


Space after if

chriseth · 2018-04-13T12:27:03Z

libsolidity/ast/Types.cpp

+namespace
+{
+
+bool fitsPrecision(bigint const _base, bigint const _exp)


Why not const&?

chriseth · 2018-04-13T13:29:15Z

libsolidity/ast/Types.cpp

+
+	size_t const bitsMax = 4096;
+
+	auto mostSignificantBit = msb(_mantissa);


Please add an explicit type.

chriseth · 2018-04-13T15:30:30Z

libsolidity/ast/Types.cpp

+namespace
+{
+
+// checks whether (_base ^ _exp) fits into 4096 bits


I think ** would be better here. Also we generally use /// ("javadoc") to document functions.

chriseth · 2018-04-13T15:30:58Z

libsolidity/ast/Types.cpp

+	if (!_base)
+		return true;
+
+	// _base > 0


Why not turn this into an assert and remove the one above?

chriseth · 2018-04-13T15:31:13Z

libsolidity/ast/Types.cpp

+
+	size_t const bitsMax = 4096;
+
+	auto mostSignificantBaseBit = msb(_base);


Please use an explicit type here.

ekpyron · 2018-04-13T17:41:41Z

OK - boost::multiprecision::msb is only introduced with boost 1.55 and trusty is still on boost 1.54 (somehow I thought it'd be at least 1.57) :-D.

I'll try to find a good workaround over the weekend.

The ci/circleci: test_x86 result Too long with no output (exceeded 10m0s) is not a good sign as well, though...

chriseth · 2018-04-16T09:08:08Z

@ekpyron were you able to find something? Otherwise I will pick this up later.

ekpyron · 2018-04-16T09:13:34Z

@chriseth I was just about to look into it. Unfortunately I think we may have to use floor(log2(...)) instead of msb again and therefore also deal with some bigfloat type... I'll check whether there's a better option.

ekpyron · 2018-04-16T09:49:43Z

@chriseth This now contains a workaround for boost < 1.55, which is a naive (but therefore sufficiently simple) implementation of msb. We have to keep an eye on the running times of the tests, though.

ekpyron · 2018-04-16T10:23:05Z

Now the ci/circleci: test_x86 test works again as well and doesn't seem to take longer than before the PR - not sure what the problem was before...

erak mentioned this pull request Mar 28, 2018

Constant folding with arbitrary precision arithmetic? #3759

Closed

axic reviewed Mar 28, 2018

View reviewed changes

axic suggested changes Mar 28, 2018

View reviewed changes

chriseth reviewed Mar 29, 2018

View reviewed changes

chriseth suggested changes Mar 29, 2018

View reviewed changes

erak self-assigned this Apr 3, 2018

erak force-pushed the rationalNumberLimit branch 5 times, most recently from 04fa673 to eeaca4a Compare April 6, 2018 16:32

ekpyron requested changes Apr 9, 2018

View reviewed changes

ekpyron reviewed Apr 9, 2018

View reviewed changes

axic reviewed Apr 9, 2018

View reviewed changes

erak force-pushed the rationalNumberLimit branch from eeaca4a to 5d11596 Compare April 10, 2018 13:00

ekpyron previously requested changes Apr 11, 2018

View reviewed changes

axic suggested changes Apr 11, 2018

View reviewed changes

argotorg deleted a comment from erak Apr 12, 2018

ekpyron force-pushed the rationalNumberLimit branch from 7cd7fd9 to fc19981 Compare April 13, 2018 09:43

ekpyron self-assigned this Apr 13, 2018

chriseth suggested changes Apr 13, 2018

View reviewed changes

ekpyron force-pushed the rationalNumberLimit branch 5 times, most recently from 788c456 to a3c08f2 Compare April 13, 2018 12:50

chriseth reviewed Apr 13, 2018

View reviewed changes

ekpyron force-pushed the rationalNumberLimit branch 10 times, most recently from fb462eb to 43cb2b0 Compare April 13, 2018 15:12

chriseth reviewed Apr 13, 2018

View reviewed changes

ekpyron force-pushed the rationalNumberLimit branch from 43cb2b0 to 93b3507 Compare April 13, 2018 15:38

chriseth suggested changes Apr 13, 2018

View reviewed changes

ekpyron force-pushed the rationalNumberLimit branch 3 times, most recently from 0573096 to 49186fb Compare April 13, 2018 16:33

ekpyron force-pushed the rationalNumberLimit branch 3 times, most recently from b9f577c to dfe8c01 Compare April 16, 2018 09:44

Limits rational numbers to 4096 bits.

33fbf88

ekpyron force-pushed the rationalNumberLimit branch from dfe8c01 to 33fbf88 Compare April 16, 2018 09:46

chriseth approved these changes Apr 16, 2018

View reviewed changes

chriseth merged commit 533d085 into develop Apr 16, 2018

axic deleted the rationalNumberLimit branch April 17, 2018 10:23


		size_t const bitsMax = 4096;

		auto mostSignificantBit = msb(_mantissa);


		size_t const bitsMax = 4096;

		auto mostSignificantBaseBit = msb(_base);

Rational number limit #3793

Rational number limit #3793

Uh oh!

Conversation

erak commented Mar 28, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erak commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Mar 29, 2018

Uh oh!

axic commented Apr 4, 2018

Uh oh!

axic commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron commented Apr 13, 2018

Uh oh!

chriseth commented Apr 16, 2018

Uh oh!

ekpyron commented Apr 16, 2018

Uh oh!

ekpyron commented Apr 16, 2018

Uh oh!

ekpyron commented Apr 16, 2018

erak commented Mar 28, 2018 •

edited by axic

Loading

erak commented Mar 28, 2018 •

edited

Loading

ekpyron Apr 11, 2018 •

edited

Loading