From 5b991c1e5983cb6efe3123495e39eca235eb7be5 Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 24 Apr 2019 14:57:31 +0200 Subject: [PATCH] Fix use of uninitialized functions stored in storage. --- Changelog.md | 1 + docs/bugs.json | 8 ++++ docs/bugs_by_version.json | 29 ++++++++++++++ libsolidity/codegen/CompilerUtils.cpp | 8 ++++ .../invalidInConstructor.sol | 23 +++++++++++ .../invalidStoredInConstructor.sol | 23 +++++++++++ .../uninitializedFunctionPointer/store2.sol | 39 +++++++++++++++++++ .../storeInConstructor.sol | 19 +++++++++ 8 files changed, 150 insertions(+) create mode 100644 test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidInConstructor.sol create mode 100644 test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidStoredInConstructor.sol create mode 100644 test/libsolidity/semanticTests/uninitializedFunctionPointer/store2.sol create mode 100644 test/libsolidity/semanticTests/uninitializedFunctionPointer/storeInConstructor.sol diff --git a/Changelog.md b/Changelog.md index 9e1ca0556..9fd38bd0b 100644 --- a/Changelog.md +++ b/Changelog.md @@ -30,6 +30,7 @@ Bugfixes: * Type System: Use correct type name for contracts in event parameters when used in libraries. This affected code generation. * Yul: Properly register functions and disallow shadowing between function variables and variables in the outside scope. + * Code Generator: Fix initialization routine of uninitialized internal function pointers in constructor context. Build System: * Soltest: Add commandline option `--test` / `-t` to isoltest which takes a string that allows filtering unit tests. diff --git a/docs/bugs.json b/docs/bugs.json index 7cf9c7adf..5bbc1d743 100644 --- a/docs/bugs.json +++ b/docs/bugs.json @@ -1,4 +1,12 @@ [ + { + "name": "UninitializedFunctionPointerInConstructor", + "summary": "Calling uninitialized internal function pointers created in the constructor does not always revert and can cause unexpected behaviour.", + "description": "Uninitialized internal function pointers point to a special piece of code that causes a revert when called. Jump target positions are different during construction and after deployment, but the code for setting this special jump target only considered the situation after deployment.", + "introduced": "0.4.5", + "fixed": "0.5.8", + "severity": "very low" + }, { "name": "IncorrectEventSignatureInLibraries", "summary": "Contract types used in events in libraries cause an incorrect event signature hash", diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json index fcdfe376e..c19d53324 100644 --- a/docs/bugs_by_version.json +++ b/docs/bugs_by_version.json @@ -380,6 +380,7 @@ }, "0.4.10": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -393,6 +394,7 @@ }, "0.4.11": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -405,6 +407,7 @@ }, "0.4.12": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -416,6 +419,7 @@ }, "0.4.13": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -427,6 +431,7 @@ }, "0.4.14": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -437,6 +442,7 @@ }, "0.4.15": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -446,6 +452,7 @@ }, "0.4.16": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -455,6 +462,7 @@ }, "0.4.17": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "EventStructWrongData", @@ -465,6 +473,7 @@ }, "0.4.18": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "EventStructWrongData", @@ -474,6 +483,7 @@ }, "0.4.19": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "ExpExponentCleanup", @@ -500,6 +510,7 @@ }, "0.4.20": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "ExpExponentCleanup", @@ -510,6 +521,7 @@ }, "0.4.21": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "ExpExponentCleanup", @@ -520,6 +532,7 @@ }, "0.4.22": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "ExpExponentCleanup", @@ -530,6 +543,7 @@ }, "0.4.23": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "ExpExponentCleanup", @@ -539,6 +553,7 @@ }, "0.4.24": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "ExpExponentCleanup", @@ -548,6 +563,7 @@ }, "0.4.25": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" ], @@ -584,6 +600,7 @@ }, "0.4.5": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -599,6 +616,7 @@ }, "0.4.6": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -613,6 +631,7 @@ }, "0.4.7": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -626,6 +645,7 @@ }, "0.4.8": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -639,6 +659,7 @@ }, "0.4.9": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ExpExponentCleanup", "NestedArrayFunctionCallDecoder", @@ -652,6 +673,7 @@ }, "0.5.0": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" ], @@ -659,6 +681,7 @@ }, "0.5.1": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" ], @@ -666,6 +689,7 @@ }, "0.5.2": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" ], @@ -673,6 +697,7 @@ }, "0.5.3": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" ], @@ -680,6 +705,7 @@ }, "0.5.4": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" ], @@ -687,6 +713,7 @@ }, "0.5.5": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "IncorrectByteInstructionOptimization", @@ -696,6 +723,7 @@ }, "0.5.6": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", "IncorrectByteInstructionOptimization" @@ -704,6 +732,7 @@ }, "0.5.7": { "bugs": [ + "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries" ], "released": "2019-03-26" diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index 1344981d5..d6be20328 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -1143,6 +1143,14 @@ void CompilerUtils::pushZeroValue(Type const& _type) m_context << m_context.lowLevelFunctionTag("$invalidFunction", 0, 0, [](CompilerContext& _context) { _context.appendInvalid(); }); + if (CompilerContext* runCon = m_context.runtimeContext()) + { + leftShiftNumberOnStack(32); + m_context << runCon->lowLevelFunctionTag("$invalidFunction", 0, 0, [](CompilerContext& _context) { + _context.appendInvalid(); + }).toSubAssemblyTag(m_context.runtimeSub()); + m_context << Instruction::OR; + } return; } } diff --git a/test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidInConstructor.sol b/test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidInConstructor.sol new file mode 100644 index 000000000..26c02f3e3 --- /dev/null +++ b/test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidInConstructor.sol @@ -0,0 +1,23 @@ +contract C { + + function() internal storedFn; + + bool flag; + + constructor() public { + if (!flag) { + flag = true; + function() internal invalid; + storedFn = invalid; + invalid(); + } + } + function f() public pure {} +} +contract Test { + function f() public { + new C(); + } +} +// ---- +// f() -> FAILURE diff --git a/test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidStoredInConstructor.sol b/test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidStoredInConstructor.sol new file mode 100644 index 000000000..710348a1a --- /dev/null +++ b/test/libsolidity/semanticTests/uninitializedFunctionPointer/invalidStoredInConstructor.sol @@ -0,0 +1,23 @@ +contract C { + + function() internal storedFn; + + bool flag; + + constructor() public { + if (!flag) { + flag = true; + function() internal invalid; + storedFn = invalid; + storedFn(); + } + } + function f() public pure {} +} +contract Test { + function f() public { + new C(); + } +} +// ---- +// f() -> FAILURE diff --git a/test/libsolidity/semanticTests/uninitializedFunctionPointer/store2.sol b/test/libsolidity/semanticTests/uninitializedFunctionPointer/store2.sol new file mode 100644 index 000000000..9d37ebed4 --- /dev/null +++ b/test/libsolidity/semanticTests/uninitializedFunctionPointer/store2.sol @@ -0,0 +1,39 @@ +pragma solidity ^0.5.7; + +contract InvalidTest { + + function() internal storedFn; + uint public x; + + constructor() public { + uint _y1; + uint _y2; + uint _y3; + uint _y4; + uint _y5; + uint _y6; + uint _y7; + uint _y8; + uint _y9; + uint _y10; + uint _y11; + uint _y12; + uint _y13; + uint _y14; + + + function() internal invalid; + storedFn = invalid; + } + + function run() public { + // this did not always cause revert in the past + storedFn(); + } + + function z() public { + x++; + } +} +// ---- +// run() -> FAILURE diff --git a/test/libsolidity/semanticTests/uninitializedFunctionPointer/storeInConstructor.sol b/test/libsolidity/semanticTests/uninitializedFunctionPointer/storeInConstructor.sol new file mode 100644 index 000000000..8a11832c8 --- /dev/null +++ b/test/libsolidity/semanticTests/uninitializedFunctionPointer/storeInConstructor.sol @@ -0,0 +1,19 @@ +contract InvalidTest { + + function() internal storedFn; + + bool flag; + + constructor() public { + function() internal invalid; + storedFn = invalid; + } + function f() public returns (uint) { + if (flag) return 2; + flag = true; + storedFn(); + } +} +// ---- +// f() -> FAILURE +// f() -> FAILURE