Do not crash on invalid index access #5096

axic · 2018-09-26T12:10:22Z

Fixes #5057.
Depends on #5195.

Another instance of this https://github.com/ethereum/solidity/blob/develop/libsolidity/analysis/TypeChecker.cpp#L2253 uses hasErrors().

I think that is unreliable and we may just use fatalError in expectType.

axic · 2018-09-26T12:12:42Z

Need to add tests.

axic · 2018-09-26T13:05:09Z

test/libsolidity/syntaxTests/indexing/array_multidim_overflow_index.sol

+    bytes[32] memory a;
+    a[8**90][8**90][1 - 8**90];
+  }
+}


This is failing with an assertion, to be fixed by this PR.

axic · 2018-09-26T13:19:41Z

test/libsolidity/syntaxTests/array/length/parameter_too_large_multidim.sol

@@ -0,0 +1,3 @@
+contract C {
+  function f(bytes32[1263941234127518272][500] memory) public pure {}
+}


This is failing with an assertion, to be fixed by this PR.

axic · 2018-09-26T13:19:48Z

test/libsolidity/syntaxTests/indexing/array_multidim_rational.sol

+    bytes[32] memory a;
+    a[8**90][8**90][8**90*0.1];
+  }
+}


This is failing with an assertion, to be fixed by this PR.

axic · 2018-09-26T13:19:55Z

test/libsolidity/syntaxTests/indexing/fixedbytes_noninteger_index.sol

+    bytes32 b;
+    b[888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888];
+  }
+}


This is failing with an assertion, to be fixed by this PR.

axic · 2018-09-26T13:49:57Z

libsolidity/analysis/TypeChecker.cpp

+			resultType = make_shared<TypeType>(make_shared<ArrayType>(
+				DataLocation::Memory,
+				typeType.actualType(),
+				length->literalValue(nullptr)


I think we are missing another test case here for too large rational numbers.

This should be already covered by e.g. array_multidim_rational.sol. Is that correct or do you have another test in mind?

axic · 2018-09-26T15:35:35Z

@chriseth please review this, the last failure is due to the need of checking calldatasize in the type checker for a multi dimensional array and rejecting if it is too large.

chriseth · 2018-09-26T20:25:02Z

libsolidity/analysis/TypeChecker.cpp

-					length->literalValue(nullptr)
-				));
-			else
+			if (!expectType(*index, IntegerType(256)) || type(*index)->category() != Type::Category::RationalNumber)


expectType already creates an error message

Ok, yeah, it was like that before, but then if we make expectType return a bool, should not the condition be negated?

codecov · 2018-10-04T18:21:48Z

Codecov Report

Merging #5096 into develop will increase coverage by <.01%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop    #5096      +/-   ##
===========================================
+ Coverage    88.02%   88.02%   +<.01%     
===========================================
  Files          314      314              
  Lines        31782    31784       +2     
  Branches      3748     3749       +1     
===========================================
+ Hits         27976    27978       +2     
  Misses        2537     2537              
  Partials      1269     1269

Flag	Coverage Δ
#all	`88.02% <77.77%> (ø)`	⬆️
#syntax	`28.78% <77.77%> (ø)`	⬆️

chriseth · 2018-10-05T12:59:40Z

libsolidity/analysis/TypeChecker.cpp

+		{
+			auto baseType = dynamic_cast<ArrayType const*>(arrayType->baseType().get());
+			if (arrayType->location() == DataLocation::Memory || arrayType->location() == DataLocation::CallData)
+				if ((baseType && !baseType->validForCalldata()) || !arrayType->validForCalldata())


If the base type is not valid for calldata, why should the type itself be? This rather sounds like the change should be inside validForCalldata

chriseth

This PR needs to be split into multiple PRs.

erak · 2018-10-10T18:14:56Z

I split out #5188 and #5195. Since these PRs are very related to each other, I based my decision on where to split them on the parts of the compiler they change and not on the test cases they cover.

This PR now depends on #5195, because the new signature of expectType() will only be introduced by that one.

erak · 2018-10-12T15:36:52Z

After checking again, it became clear that #5195 already fixed the remaining bug that this PR is trying to fix. I will close it now as this change does not seem to be needed anymore.

axic changed the title ~~Do not crash on invalid indexed access~~ [WIP] Do not crash on invalid indexed access Sep 26, 2018

axic force-pushed the invalid-index-literals branch from fc34789 to af92198 Compare September 26, 2018 13:00

axic commented Sep 26, 2018

View reviewed changes

axic force-pushed the invalid-index-literals branch from af92198 to e298c8e Compare September 26, 2018 13:19

axic commented Sep 26, 2018

View reviewed changes

axic force-pushed the invalid-index-literals branch from e298c8e to 2ac25ea Compare September 26, 2018 13:48

axic commented Sep 26, 2018

View reviewed changes

chriseth reviewed Sep 26, 2018

View reviewed changes

chriseth previously approved these changes Sep 26, 2018

View reviewed changes

erak self-assigned this Oct 1, 2018

erak dismissed chriseth’s stale review via a68b481 October 4, 2018 17:21

axic and others added 3 commits October 4, 2018 19:35

Add comprehensive tests for invalid array indexing

ebf5035

Do not crash on invalid indexed access

507cdb8

Reports error if multidim. array is too large.

851596a

erak force-pushed the invalid-index-literals branch from a68b481 to 851596a Compare October 4, 2018 17:36

Updates Changelog.

0fd1ea6

erak changed the title ~~[WIP] Do not crash on invalid indexed access~~ Do not crash on invalid indexed access Oct 4, 2018

erak changed the title ~~Do not crash on invalid indexed access~~ Do not crash on invalid index access Oct 4, 2018

chriseth reviewed Oct 5, 2018

View reviewed changes

chriseth suggested changes Oct 5, 2018

View reviewed changes

erak added 2 commits October 10, 2018 18:37

Split out fix for too large arrays.

d617a5a

Splits out fix for non-unsigned array index.

c12e9e7

erak mentioned this pull request Oct 10, 2018

More array index tests #5197

Merged

Split out syntax tests.

838e4d1

erak closed this Oct 12, 2018

axic deleted the invalid-index-literals branch March 22, 2019 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not crash on invalid index access #5096

Do not crash on invalid index access #5096

axic commented Sep 26, 2018 •

edited by erak

Loading

axic commented Sep 26, 2018

axic Sep 26, 2018

erak Oct 4, 2018

axic Sep 26, 2018

erak Oct 4, 2018

axic Sep 26, 2018

erak Oct 4, 2018

axic Sep 26, 2018

erak Oct 4, 2018

axic Sep 26, 2018

erak Oct 4, 2018

axic commented Sep 26, 2018

chriseth Sep 26, 2018

chriseth Sep 26, 2018

codecov bot commented Oct 4, 2018 •

edited

Loading

chriseth Oct 5, 2018

chriseth left a comment

erak commented Oct 10, 2018

erak commented Oct 12, 2018

Do not crash on invalid index access #5096

Do not crash on invalid index access #5096

Conversation

axic commented Sep 26, 2018 • edited by erak Loading

axic commented Sep 26, 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

axic commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

chriseth left a comment

Choose a reason for hiding this comment

erak commented Oct 10, 2018

erak commented Oct 12, 2018

axic commented Sep 26, 2018 •

edited by erak

Loading

codecov bot commented Oct 4, 2018 •

edited

Loading