Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when the storage layout base is near the end of storage (2^64 slots or less) #15912

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

Language Features:
* Introduce syntax for specifying contract storage layout base.
* Code Generator: generate code for contracts with a specified storage layout base.


Compiler Features:
Expand Down
41 changes: 31 additions & 10 deletions libsolidity/analysis/PostTypeContractLevelChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,17 @@ bool PostTypeContractLevelChecker::check(ContractDefinition const& _contract)
errorHashes[hash][signature] = error->location();
}

if (auto const* layoutSpecifier = _contract.storageLayoutSpecifier())
checkStorageLayoutSpecifier(*layoutSpecifier);
if (_contract.storageLayoutSpecifier())
checkStorageLayoutSpecifier(_contract);

return !Error::containsErrors(m_errorReporter.errors());
}

void PostTypeContractLevelChecker::checkStorageLayoutSpecifier(StorageLayoutSpecifier const& _storageLayoutSpecifier)
void PostTypeContractLevelChecker::checkStorageLayoutSpecifier(ContractDefinition const& _contract)
{
Expression const& baseSlotExpression = _storageLayoutSpecifier.baseSlotExpression();
StorageLayoutSpecifier const* storageLayoutSpecifier = _contract.storageLayoutSpecifier();
solAssert(storageLayoutSpecifier);
Expression const& baseSlotExpression = storageLayoutSpecifier->baseSlotExpression();

if (!*baseSlotExpression.annotation().isPure)
{
Expand Down Expand Up @@ -117,22 +119,41 @@ void PostTypeContractLevelChecker::checkStorageLayoutSpecifier(StorageLayoutSpec
}
solAssert(rationalType->value().denominator() == 1);

if (
rationalType->value().numerator() < 0 ||
rationalType->value().numerator() > std::numeric_limits<u256>::max()
)
bigint baseSlot = rationalType->value().numerator();
if (!(0 <= baseSlot && baseSlot <= std::numeric_limits<u256>::max()))
{
m_errorReporter.typeError(
6753_error,
baseSlotExpression.location(),
fmt::format(
"The base slot of the storage layout evaluates to {}, which is outside the range of type uint256.",
formatNumberReadable(rationalType->value().numerator())
formatNumberReadable(baseSlot)
)
);
return;
}

solAssert(baseSlotExpressionType->isImplicitlyConvertibleTo(*TypeProvider::uint256()));
_storageLayoutSpecifier.annotation().baseSlot = u256(rationalType->value().numerator());
storageLayoutSpecifier->annotation().baseSlot = u256(baseSlot);

if (
u256 slotsLeft = std::numeric_limits<u256>::max() - *storageLayoutSpecifier->annotation().baseSlot;
slotsLeft <= u256(1) << 64
)
Comment on lines +139 to +142
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think of it, this check is not really limited to contracts with a layout. The dangerous thing is leaving too few slots between the last storage variable and the storage end, regardless of the base slot. It's more likely to happen if you have a non-zero base slot, but technically, you could end up in that situation also by just having large enough variables. And you should get this warning even if you're already getting the one for oversized variables because they warn about different issues.

Let's do this:

  • Check base slot + size rather than just base slot.
  • Perform the check for both storage and transient storage.
  • Do this check even if the layout specifier is not present.
    • If the contract has no layout specifier, point at the contract itself in the error.
    • If contract has storage/transient variables, point at the last one, saying how close that variable is to the edge.
    • Do it only if there are no previous errors reported. This way we won't be unnecessarily reporting it when the base slot + size goes past the edge and the contract won't compile anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, not sure about transient storage. There are no upgradability issues there so I wonder if we should not warn there or if the fact that this is so close to the edge still warrants a warning...

m_errorReporter.warning(
3495_error,
storageLayoutSpecifier->baseSlotExpression().location(),
fmt::format(
"There are {} slots before the end of the contract storage when this specified base layout is used.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

Suggested change
"There are {} slots before the end of the contract storage when this specified base layout is used.",
"This contract is very close to the end of storage. This limits its future upgradability.",

Like I said above, I'd put the number of slots left in the secondary message, pointing at a specific variable.

formatNumberReadable(slotsLeft)
));

bigint size = contractStorageSizeUpperBound(_contract, VariableDeclaration::Location::Unspecified);
solAssert(size < bigint(1) << 256);
if (baseSlot + size >= bigint(1) << 256)
m_errorReporter.typeError(
5015_error,
baseSlotExpression.location(),
"Contract extends past the end of storage when this base slot value is specified."
);
}
2 changes: 1 addition & 1 deletion libsolidity/analysis/PostTypeContractLevelChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class PostTypeContractLevelChecker
/// @returns true iff all checks passed. Note even if all checks passed, errors() can still contain warnings
bool check(ContractDefinition const& _contract);

void checkStorageLayoutSpecifier(StorageLayoutSpecifier const& _storageLayoutSpecifier);
void checkStorageLayoutSpecifier(ContractDefinition const& _contract);

langutil::ErrorReporter& m_errorReporter;
};
Expand Down
14 changes: 14 additions & 0 deletions libsolidity/ast/ASTUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,18 @@ bigint contractStorageSizeUpperBound(ContractDefinition const& _contract, Variab
return size;
}

u256 layoutBaseForInheritanceHierarchy(ContractDefinition const& _topLevelContract, DataLocation _location)
{
if (_location != DataLocation::Storage)
{
solAssert(_location == DataLocation::Transient);
return 0;
}

if (auto const* storageLayoutSpecifier = _topLevelContract.storageLayoutSpecifier())
return *storageLayoutSpecifier->annotation().baseSlot;

return 0;
}

}
9 changes: 9 additions & 0 deletions libsolidity/ast/ASTUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

#pragma once

#include <libsolutil/Numeric.h>

namespace solidity::frontend
{

Expand All @@ -26,6 +28,7 @@ class Declaration;
class Expression;
class SourceUnit;
class VariableDeclaration;
class ContractDefinition;

/// Find the topmost referenced constant variable declaration when the given variable
/// declaration value is an identifier. Works only for constant variable declarations.
Expand All @@ -52,4 +55,10 @@ Type const* type(VariableDeclaration const& _variable);
/// located in @a _location (either storage or transient storage).
bigint contractStorageSizeUpperBound(ContractDefinition const& _contract, VariableDeclaration::Location _location);

/// @returns The base slot of the inheritance hierarchy rooted at the specified contract.
/// The value comes from the inheritance specifier of the contract and defaults to zero.
/// The value is zero also when the contract is an interface or a library (and cannot have storage).
/// Assumes analysis was successful.
u256 layoutBaseForInheritanceHierarchy(ContractDefinition const& _topLevelContract, DataLocation _location);

}
20 changes: 12 additions & 8 deletions libsolidity/ast/Types.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <libsolidity/ast/Types.h>

#include <libsolidity/ast/AST.h>
#include <libsolidity/ast/ASTUtils.h>
#include <libsolidity/ast/TypeProvider.h>

#include <libsolidity/analysis/ConstantEvaluator.h>
Expand Down Expand Up @@ -138,23 +139,24 @@ void Type::clearCache() const
m_stackSize.reset();
}

void StorageOffsets::computeOffsets(TypePointers const& _types)
void StorageOffsets::computeOffsets(TypePointers const& _types, u256 _baseSlot)
{
bigint slotOffset = 0;
bigint slotOffset = bigint(_baseSlot);
bigint storageSize = 0;
unsigned byteOffset = 0;
std::map<size_t, std::pair<u256, unsigned>> offsets;
for (size_t i = 0; i < _types.size(); ++i)
{
Type const* type = _types[i];
if (!type->canBeStored())
continue;
solAssert(type->canBeStored());
solAssert(type->storageBytes() <= 32);
if (byteOffset + type->storageBytes() > 32)
{
// would overflow, go to next slot
++slotOffset;
byteOffset = 0;
}
solAssert(slotOffset < bigint(1) << 256 ,"Object too large for storage.");
solAssert(slotOffset < bigint(1) << 256, "Object extends past the end of storage.");
offsets[i] = std::make_pair(u256(slotOffset), byteOffset);
solAssert(type->storageSize() >= 1, "Invalid storage size.");
if (type->storageSize() == 1 && byteOffset + type->storageBytes() <= 32)
Expand All @@ -167,8 +169,9 @@ void StorageOffsets::computeOffsets(TypePointers const& _types)
}
if (byteOffset > 0)
++slotOffset;
solAssert(slotOffset < bigint(1) << 256, "Object too large for storage.");
m_storageSize = u256(slotOffset);

solAssert(slotOffset < bigint(1) << 256, "Object extends past the end of storage.");
m_storageSize = u256(slotOffset - _baseSlot);
swap(m_offsets, offsets);
}

Expand Down Expand Up @@ -2168,11 +2171,12 @@ std::vector<std::tuple<VariableDeclaration const*, u256, unsigned>> ContractType
for (VariableDeclaration const* variable: contract->stateVariables())
if (!(variable->isConstant() || variable->immutable()) && variable->referenceLocation() == location)
variables.push_back(variable);

TypePointers types;
for (auto variable: variables)
types.push_back(variable->annotation().type);
StorageOffsets offsets;
offsets.computeOffsets(types);
offsets.computeOffsets(types, layoutBaseForInheritanceHierarchy(m_contract, _location));

std::vector<std::tuple<VariableDeclaration const*, u256, unsigned>> variablesAndOffsets;
for (size_t index = 0; index < variables.size(); ++index)
Expand Down
5 changes: 4 additions & 1 deletion libsolidity/ast/Types.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ class StorageOffsets
public:
/// Resets the StorageOffsets objects and determines the position in storage for each
/// of the elements of @a _types.
void computeOffsets(TypePointers const& _types);
/// Calculated positions are absolute and start at @a _baseSlot.
/// Assumes that @a _types is small enough to fit in the area between @a _baseSlot and the end of storage
/// (the caller is responsible for validating that).
void computeOffsets(TypePointers const& _types, u256 _baseSlot = 0);
/// @returns the offset of the given member, might be null if the member is not part of storage.
std::pair<u256, unsigned> const* offset(size_t _index) const;
/// @returns the total number of slots occupied by all members.
Expand Down
3 changes: 0 additions & 3 deletions libsolidity/interface/CompilerStack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,6 @@ Json const& CompilerStack::storageLayout(Contract const& _contract) const
solAssert(m_stackState >= AnalysisSuccessful, "Analysis was not successful.");
solAssert(_contract.contract);
solUnimplementedAssert(!isExperimentalSolidity());
solUnimplementedAssert(!_contract.contract->storageLayoutSpecifier(), "Storage layout not supported for contract with specified layout base.");

return _contract.storageLayout.init([&]{ return StorageLayout().generate(*_contract.contract, DataLocation::Storage); });
}
Expand Down Expand Up @@ -1534,7 +1533,6 @@ void CompilerStack::compileContract(
solAssert(!m_viaIR, "");
solUnimplementedAssert(!m_eofVersion.has_value(), "Experimental EOF support is only available for via-IR compilation.");
solAssert(m_stackState >= AnalysisSuccessful, "");
solUnimplementedAssert(!_contract.storageLayoutSpecifier(), "Code generation is not supported for contracts with specified storage layout base.");

if (_otherCompilers.count(&_contract))
return;
Expand Down Expand Up @@ -1570,7 +1568,6 @@ void CompilerStack::compileContract(
void CompilerStack::generateIR(ContractDefinition const& _contract, bool _unoptimizedOnly)
{
solAssert(m_stackState >= AnalysisSuccessful, "");
solUnimplementedAssert(!_contract.storageLayoutSpecifier(), "Code generation is not supported for contracts with specified storage layout base.");
Contract& compiledContract = m_contracts.at(_contract.fullyQualifiedName());
if (compiledContract.yulIR)
{
Expand Down

This file was deleted.

5 changes: 0 additions & 5 deletions test/cmdlineTests/storage_layout_specifier_codegen_error/err

This file was deleted.

This file was deleted.

This file was deleted.

1 change: 1 addition & 0 deletions test/cmdlineTests/storage_layout_specifier_ir_codegen/args
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--optimize --ir-optimized --debug-info none -
20 changes: 20 additions & 0 deletions test/cmdlineTests/storage_layout_specifier_ir_codegen/output
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
Optimized IR:
/// @use-src 0:"<stdin>"
object "C_7" {
code {
{
let _1 := memoryguard(0x80)
mstore(64, _1)
if callvalue() { revert(0, 0) }
sstore(0x2a, 0x0a)
let _2 := datasize("C_7_deployed")
codecopy(_1, dataoffset("C_7_deployed"), _2)
return(_1, _2)
}
}
/// @use-src 0:"<stdin>"
object "C_7_deployed" {
code { { revert(0, 0) } }
data ".metadata" hex"<BYTECODE REMOVED>"
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
//SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

contract C layout at 42 {}
contract C layout at 42 {
uint x = 10;
}

This file was deleted.

This file was deleted.

This file was deleted.

1 change: 1 addition & 0 deletions test/cmdlineTests/storage_layout_specifier_smoke/args
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--no-cbor-metadata --optimize --asm --debug-info none -
85 changes: 85 additions & 0 deletions test/cmdlineTests/storage_layout_specifier_smoke/output
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@

======= <stdin>:C =======
EVM assembly:
mstore(0x40, 0x80)
callvalue
dup1
iszero
tag_1
jumpi
revert(0x00, 0x00)
tag_1:
pop
dataSize(sub_0)
dup1
dataOffset(sub_0)
0x00
codecopy
0x00
return
stop

sub_0: assembly {
mstore(0x40, 0x80)
callvalue
dup1
iszero
tag_1
jumpi
revert(0x00, 0x00)
tag_1:
pop
jumpi(tag_2, lt(calldatasize, 0x04))
shr(0xe0, calldataload(0x00))
dup1
0x26121ff0
eq
tag_3
jumpi
tag_2:
revert(0x00, 0x00)
tag_3:
tag_4
tag_5
jump // in
tag_4:
stop
tag_5:
sload(0x0abc)
tag_7
swap1
0x01
tag_8
jump // in
tag_7:
0x0abc
sstore
jump // out
tag_8:
dup1
dup3
add
dup1
dup3
gt
iszero
tag_11
jumpi
0x4e487b71
0xe0
shl
0x00
mstore
0x11
0x04
mstore
0x24
0x00
revert
tag_11:
swap3
swap2
pop
pop
jump // out
}
9 changes: 9 additions & 0 deletions test/cmdlineTests/storage_layout_specifier_smoke/stdin
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0.0;

contract C layout at 0xABC {
uint x;
function f() public {
x = x + 1;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--storage-layout --pretty-json --json-indent 4 -
Loading