Merge pull request ethereum#10024 from ethereum/outofBoundsGetter

Use revert for out-of-bounds array index access in getter.
sgilflores3 · Oct 14, 2020 · 92a2cdd · 92a2cdd
2 parents 9b1f905 + 5dc3a97
commit 92a2cdd
Show file tree

Hide file tree

Showing 11 changed files with 40 additions and 23 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -14,6 +14,7 @@ Bugfixes:
  * Code generator: Fix internal error on returning structs containing mappings from library function.
  * Code generator: Fix internal compiler error when referencing members via module name but not using the reference.
  * Code generator: Fix ``ABIEncoderV2`` pragma from the current module affecting inherited functions and applied modifiers.
+ * Code generator: Use revert instead of invalid opcode for out-of-bounds array index access in getter.
  * Type Checker: Fix internal compiler error caused by storage parameters with nested mappings in libraries.
  * Name Resolver: Fix shadowing/same-name warnings for later declarations.
  * Contract Level Checker: Add missing check against inheriting functions with ABIEncoderV2 return types in ABIEncoderV1 contracts.

diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp
@@ -170,7 +170,16 @@ void ExpressionCompiler::appendStateVariableAccessor(VariableDeclaration const&
 			// pop offset
 			m_context << Instruction::POP;
 			utils().copyToStackTop(paramTypes.size() - i + 1, 1);
-			ArrayUtils(m_context).accessIndex(*arrayType);
+
+			ArrayUtils(m_context).retrieveLength(*arrayType, 1);
+			// Stack: ref [length] index length
+			// check out-of-bounds access
+			m_context << Instruction::DUP2 << Instruction::LT;
+			auto tag = m_context.appendConditionalJump();
+			m_context << u256(0) << Instruction::DUP1 << Instruction::REVERT;
+			m_context << tag;
+
+			ArrayUtils(m_context).accessIndex(*arrayType, false);
 			returnType = arrayType->baseType();
 		}
 		else

diff --git a/libsolidity/codegen/ir/IRGenerator.cpp b/libsolidity/codegen/ir/IRGenerator.cpp
@@ -348,18 +348,25 @@ string IRGenerator::generateGetter(VariableDeclaration const& _varDecl)
 				mappingType ? *mappingType->keyType() : *TypeProvider::uint256()
 			).stackSlots();
 			parameters += keys;
-			code += Whiskers(R"(
+
+			Whiskers templ(R"(
+				<?array>
+					if iszero(lt(<keys>, <length>(slot))) { revert(0, 0) }
+				</array>
 				slot<?array>, offset</array> := <indexAccess>(slot<?+keys>, <keys></+keys>)
-			)")
-			(
+			)");
+			templ(
 				"indexAccess",
 				mappingType ?
 				m_utils.mappingIndexAccessFunction(*mappingType, *mappingType->keyType()) :
 				m_utils.storageArrayIndexAccessFunction(*arrayType)
 			)
 			("array", arrayType != nullptr)
-			("keys", joinHumanReadable(keys))
-			.render();
+			("keys", joinHumanReadable(keys));
+			if (arrayType)
+				templ("length", m_utils.arrayLengthFunction(*arrayType));
+
+			code += templ.render();
 
 			currentType = mappingType ? mappingType->valueType() : arrayType->baseType();
 		}

diff --git a/test/libsolidity/gasTests/abiv2.sol b/test/libsolidity/gasTests/abiv2.sol
@@ -14,9 +14,9 @@ contract C {
 }
 // ----
 // creation:
-//   codeDepositCost: 1106800
-//   executionCost: 1147
-//   totalCost: 1107947
+//   codeDepositCost: 1107400
+//   executionCost: 1154
+//   totalCost: 1108554
 // external:
 //   a(): 1130
 //   b(uint256): infinite

diff --git a/test/libsolidity/gasTests/abiv2_optimised.sol b/test/libsolidity/gasTests/abiv2_optimised.sol
@@ -17,9 +17,9 @@ contract C {
 // optimize-yul: true
 // ----
 // creation:
-//   codeDepositCost: 604400
+//   codeDepositCost: 605000
 //   executionCost: 638
-//   totalCost: 605038
+//   totalCost: 605638
 // external:
 //   a(): 1029
 //   b(uint256): 2084

diff --git a/test/libsolidity/gasTests/dispatch_large.sol b/test/libsolidity/gasTests/dispatch_large.sol
@@ -24,9 +24,9 @@ contract Large {
 }
 // ----
 // creation:
-//   codeDepositCost: 637000
+//   codeDepositCost: 637600
 //   executionCost: 670
-//   totalCost: 637670
+//   totalCost: 638270
 // external:
 //   a(): 1051
 //   b(uint256): 2046

diff --git a/test/libsolidity/gasTests/dispatch_large_optimised.sol b/test/libsolidity/gasTests/dispatch_large_optimised.sol
@@ -27,9 +27,9 @@ contract Large {
 // optimize-runs: 2
 // ----
 // creation:
-//   codeDepositCost: 260600
+//   codeDepositCost: 261200
 //   executionCost: 300
-//   totalCost: 260900
+//   totalCost: 261500
 // external:
 //   a(): 998
 //   b(uint256): 2305

diff --git a/test/libsolidity/gasTests/dispatch_medium.sol b/test/libsolidity/gasTests/dispatch_medium.sol
@@ -11,9 +11,9 @@ contract Medium {
 }
 // ----
 // creation:
-//   codeDepositCost: 253200
+//   codeDepositCost: 253800
 //   executionCost: 294
-//   totalCost: 253494
+//   totalCost: 254094
 // external:
 //   a(): 1028
 //   b(uint256): 2046

diff --git a/test/libsolidity/gasTests/dispatch_medium_optimised.sol b/test/libsolidity/gasTests/dispatch_medium_optimised.sol
@@ -14,9 +14,9 @@ contract Medium {
 // optimize-runs: 2
 // ----
 // creation:
-//   codeDepositCost: 141000
+//   codeDepositCost: 141600
 //   executionCost: 190
-//   totalCost: 141190
+//   totalCost: 141790
 // external:
 //   a(): 998
 //   b(uint256): 2063

diff --git a/test/libsolidity/gasTests/dispatch_small.sol b/test/libsolidity/gasTests/dispatch_small.sol
@@ -6,9 +6,9 @@ contract Small {
 }
 // ----
 // creation:
-//   codeDepositCost: 84800
+//   codeDepositCost: 85400
 //   executionCost: 135
-//   totalCost: 84935
+//   totalCost: 85535
 // external:
 //   fallback: 129
 //   a(): 983

diff --git a/test/libsolidity/gasTests/dispatch_small_optimised.sol b/test/libsolidity/gasTests/dispatch_small_optimised.sol
@@ -9,9 +9,9 @@ contract Small {
 // optimize-runs: 2
 // ----
 // creation:
-//   codeDepositCost: 60600
+//   codeDepositCost: 61200
 //   executionCost: 111
-//   totalCost: 60711
+//   totalCost: 61311
 // external:
 //   fallback: 118
 //   a(): 976