Assembly clearing local variable #13494

wighawag · 2022-09-07T09:12:51Z

Description

It seems in some conditions, assembly code is messing up with local variable even when it should not

What is even weirder, is that adding some extra code remove the issue.

Reproduction repo here :
https://github.com/bug-reproduction/solidity-assembly-variable-reset

Environment

Compiler version: 0.8.16 (tested also on 0.8.13)
Target EVM version (as per compiler settings): default settings with optimizer (runs: 2000)
Framework/IDE (e.g. Truffle or Remix): hardhat
EVM execution environment / backend / blockchain client: geth
Operating system: linux

Steps to Reproduce

// SPDX-License-Identifier: AGPL-1.0
pragma solidity 0.8.16;

contract Test {
	event Debug(address deployedContract);

	function deployData(bytes calldata) external {
		address newContract;
		assembly {
			let len := calldataload(36)
			let p := mload(0x40)
			mstore(p, 0x61FFFF600E60003961FFFF6000F3000000000000000000000000000000000000)
			let lenByte1 := shr(8, len)
			let lenByte2 := and(len, 0xFF)
			mstore8(add(p, 1), lenByte1)
			mstore8(add(p, 2), lenByte2)
			mstore8(add(p, 9), lenByte1)
			mstore8(add(p, 10), lenByte2)
			calldatacopy(add(p, 14), 68, len)

			newContract := create(0, p, add(len, 14))
			// log1(p, add(len, 14), newContract) // need this line, no idea why, looks like some memory management issues
		}
		emit Debug(newContract);
	}

	function deployDataWithLog1(bytes calldata) external {
		address newContract;
		assembly {
			let len := calldataload(36)
			let p := mload(0x40)
			mstore(p, 0x61FFFF600E60003961FFFF6000F3000000000000000000000000000000000000)
			let lenByte1 := shr(8, len)
			let lenByte2 := and(len, 0xFF)
			mstore8(add(p, 1), lenByte1)
			mstore8(add(p, 2), lenByte2)
			mstore8(add(p, 9), lenByte1)
			mstore8(add(p, 10), lenByte2)
			calldatacopy(add(p, 14), 68, len)

			newContract := create(0, p, add(len, 14))
			log1(p, add(len, 14), newContract) // need this line, no idea why, looks like some memory management issues
		}
		emit Debug(newContract);
	}

	function deployData2(bytes calldata data) external {
		bytes memory deployCode = bytes.concat(hex"61FFFF600E60003961FFFF6000F3", data);
		bytes1 lenByte1 = bytes1(uint8(data.length >> 8));
		bytes1 lenByte2 = bytes1(uint8(data.length & 0xFF));
		deployCode[1] = lenByte1;
		deployCode[9] = lenByte1;
		deployCode[2] = lenByte2;
		deployCode[10] = lenByte2;

		address newContract;
		assembly {
			newContract := create(0, add(deployCode, 32), mload(deployCode))
			// log1(add(deployCode, 32), mload(deployCode), newContract) // need this line, no idea why, looks like some memory management issues
		}
		emit Debug(newContract);
	}

	function deployData2WithLog1(bytes calldata data) external {
		bytes memory deployCode = bytes.concat(hex"61FFFF600E60003961FFFF6000F3", data);
		bytes1 lenByte1 = bytes1(uint8(data.length >> 8));
		bytes1 lenByte2 = bytes1(uint8(data.length & 0xFF));
		deployCode[1] = lenByte1;
		deployCode[9] = lenByte1;
		deployCode[2] = lenByte2;
		deployCode[10] = lenByte2;

		address newContract;
		assembly {
			newContract := create(0, add(deployCode, 32), mload(deployCode))
			log1(add(deployCode, 32), mload(deployCode), newContract) // need this line, no idea why, looks like some memory management issues
		}
		emit Debug(newContract);
	}
}

The issue only arise when the input data is big enough, tested with 24000 bytes

see repo for reproducing the bug : https://github.com/bug-reproduction/solidity-assembly-variable-reset

The text was updated successfully, but these errors were encountered:

hrkrshnn · 2022-09-07T11:48:36Z

It seems in some conditions, assembly code is messing up with local variable even when it should not

What do you mean by this? @wighawag

ekpyron · 2022-09-07T12:45:24Z

Hm... first I thought, I can't reproduce it, but the following isoltest test actually fails in an unoptimized via-IR run on the second event emission...

pragma solidity ^0.8.0;

contract Test {
	event Debug(address deployedContract);

	function deployData(bytes calldata) external {
		address newContract;
		assembly {
			let len := calldataload(36)
			let p := mload(0x40)
			mstore(p, 0x61FFFF600E60003961FFFF6000F3000000000000000000000000000000000000)
			let lenByte1 := shr(8, len)
			let lenByte2 := and(len, 0xFF)
			mstore8(add(p, 1), lenByte1)
			mstore8(add(p, 2), lenByte2)
			mstore8(add(p, 9), lenByte1)
			mstore8(add(p, 10), lenByte2)
			calldatacopy(add(p, 14), 68, len)

			newContract := create(0, p, add(len, 14))
			// log1(p, add(len, 14), newContract) // need this line, no idea why, looks like some memory management issues
		}
		emit Debug(newContract);
	}

	function deployData2(bytes calldata data) external {
		bytes memory deployCode = bytes.concat(hex"61FFFF600E60003961FFFF6000F3", data);
		bytes1 lenByte1 = bytes1(uint8(data.length >> 8));
		bytes1 lenByte2 = bytes1(uint8(data.length & 0xFF));
		deployCode[1] = lenByte1;
		deployCode[9] = lenByte1;
		deployCode[2] = lenByte2;
		deployCode[10] = lenByte2;

		address newContract;
		assembly {
			newContract := create(0, add(deployCode, 32), mload(deployCode))
			// log1(add(deployCode, 32), mload(deployCode), newContract) // need this line, no idea why, looks like some memory management issues
		}
		emit Debug(newContract);
	}
	function test() external {
        bytes memory x = new bytes(1024*32);
        x[0] = 0x23;
        for (uint i = 1; i < x.length; ++i) {
            x[i] = bytes1(uint8((uint(uint8(x[i-1])) * 17) % 7));
        }
        this.deployData(x);
        this.deployData2(x);
    }
}
// ----
// test() ->
// ~ emit Debug(address): 0xf01f7809444bd9a93a854361c6fae3f23d9e23db
// ~ emit Debug(address): 0x7963f166e7cf4cb8af121bbabfefb078e9e5ddb8
// gas irOptimized: 26055073
// gas legacy: 44269004
// gas legacyOptimized: 22321659

i.e. the second event emission emits a zero address in the unoptimized via-IR run.

Interestingly, though, the same happens when exchanging this.deployData with this.deployData2, i.e. it's still the second emission affected only...

The legacy runs and optimized runs also seem fine... and the IR code looks rather innocent at least at a quick glance, though, so I'm expecting this to be some kind of false positive but it's suspicious that something similar seems to happen in our testing framework and with hardhat, so there's definitely cause to investigate this...

wighawag · 2022-09-07T13:19:17Z

@hrkrshnn

What do you mean by this? @wighawag

address newContract; should be non-zero after newContract := create(0, p, add(len, 14))
but it is zero when calling emit Debug(newContract);

if you uncomment the log1 line it becomes correct

so not sure of the issue is that newContract got affected or as @ekpyron mention something else is happening, but there something going wrong here.

r0qs · 2022-09-07T15:03:37Z

Hi @wighawag, the test fails with out-of-gas when running the repo that you mentioned in the issue. It is a bit strange that the log1 would prevent that.

Running your code on foundry though seems to work fine, with or without the log1:

Running 1 test for test/Test.sol:Test
[PASS] test() (gas: 9273752)
Traces:
  [9273752] Test::test() 
    ├─ [4757006] Test::deployData(0x615870600e6000396158706000f3504852706447786c50...
    │   ├─ emit Debug(deployedContract: 0xce71065d4017f316ec606fe4422e11eb2c47c246)
    ├─ [4575427] Test::deployData2(0x615870600e6000396158706000f3504852706447786c50...
    │   ├─ emit Debug(deployedContract: 0x185a4dc360ce69bdccee33b3784b0282f7961aea)

I also tested using the example payload that you are using in your tests. Could you please provide some more information about the bug?

wighawag · 2022-09-07T15:09:01Z

@r0qs are you using the geth node provided in the repo via docker-compose ?

the run out of gas issue happen on hardhat, running against geth expose the issue and where if you uncomment the log1 it passes

ekpyron · 2022-09-07T15:17:48Z

FYI the second creation failing with out-of-gas, resulting in create returning zero, fully explains what I saw in #13494 (comment) - but I hadn't looked into the logging part and let @r0qs take over the investigation for now. But yeah, sounds weird that adding the logging should prevent out-of-gas somehow...

wighawag · 2022-09-07T15:37:45Z

@r0qs you might have forgotten that step if you used geth : pnpm execute localhost scripts/fundingFromCoinbase.ts

I know this sounds crazy and you made me tripple check what I saw, but it really is happening, see CI action : https://github.com/bug-reproduction/solidity-assembly-variable-reset/runs/8231871860?check_suite_focus=true

I add the example with the extra log1 and it passes

r0qs · 2022-09-07T16:00:58Z

@ekpyron compiling the code with solc and --via-ir gives segmentation fault in solidity::yul::Parser::parseSrcComment

#80515 0x0000555555c546ee in bool std::__detail::__regex_algo_impl<char const*, std::allocator<std::__cxx11::sub_match<char const*> >, char, std::__cxx11::regex_traits<char> >(char const*, char const*, std::__cxx11::match_results<char const*, std::allocator<std::__cxx11::sub_match<char const*> > >&, std::__cxx11::basic_regex<char, std::__cxx11::regex_traits<char> > const&, std::regex_constants::match_flag_type, std::__detail::_RegexExecutorPolicy, bool) ()
#80516 0x0000555555c4c3f4 in solidity::yul::Parser::parseSrcComment(std::basic_string_view<char, std::char_traits<char> >, solidity::langutil::SourceLocation const&) ()
#80517 0x0000555555c4d63a in solidity::yul::Parser::fetchDebugDataFromComment() ()
#80518 0x0000555555c4d91c in solidity::yul::Parser::advance() ()
#80519 0x0000555555c50e09 in solidity::yul::Parser::parseBlock() ()
#80520 0x0000555555c516e8 in solidity::yul::Parser::parseFunctionDefinition() ()
#80521 0x0000555555c4ff88 in solidity::yul::Parser::parseStatement() ()
#80522 0x0000555555c50e66 in solidity::yul::Parser::parseBlock() ()
#80523 0x0000555555c51a83 in solidity::yul::Parser::parseInline(std::shared_ptr<solidity::langutil::Scanner> const&) ()
#80524 0x0000555555c6e801 in solidity::yul::ObjectParser::parseBlock(std::optional<std::map<unsigned int, std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const> > > > >) ()
#80525 0x0000555555c6ecaf in solidity::yul::ObjectParser::parseCode(std::optional<std::map<unsigned int, std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const> > > > >) ()
#80526 0x0000555555c715e1 in solidity::yul::ObjectParser::parseObject(solidity::yul::Object*) ()
#80527 0x0000555555c71985 in solidity::yul::ObjectParser::parseObject(solidity::yul::Object*) ()
#80528 0x0000555555c71ba9 in solidity::yul::ObjectParser::parse(std::shared_ptr<solidity::langutil::Scanner> const&, bool) ()
#80529 0x0000555555c5ec20 in solidity::yul::YulStack::parseAndAnalyze(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#80530 0x0000555555aa0e24 in solidity::frontend::IRGenerator::run[abi:cxx11](solidity::frontend::ContractDefinition const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::map<solidity::frontend::ContractDefinition const*, std::basic_string_view<char, std::char_traits<char> > const, std::less<solidity::frontend::ContractDefinition const*>, std::allocator<std::pair<solidity::frontend::ContractDefinition const* const, std::basic_string_view<char, std::char_traits<char> > const> > > const&) ()
#80531 0x00005555558048d1 in solidity::frontend::CompilerStack::generateIR(solidity::frontend::ContractDefinition const&) ()
#80532 0x000055555580929b in solidity::frontend::CompilerStack::compile(solidity::frontend::CompilerStack::State) ()
#80533 0x0000555555735c26 in solidity::frontend::CommandLineInterface::compile() ()
#80534 0x000055555573b015 in solidity::frontend::CommandLineInterface::processInput() ()
#80535 0x000055555573b40c in solidity::frontend::CommandLineInterface::run(int, char const* const*) ()
#80536 0x0000555555706e6c in main ()

Maybe something related with this regex and comments inside an assembly block: https://github.com/ethereum/solidity/blob/develop/libyul/AsmParser.cpp#L194

solc version: Version: 0.8.17-develop.2022.9.5+commit.51929652.Linux.g++

r0qs · 2022-09-07T16:11:19Z

@r0qs you might have forgotten that step if you used geth : pnpm execute localhost scripts/fundingFromCoinbase.ts

I know this sounds crazy and you made me tripple check what I saw, but it really is happening, see CI action : https://github.com/bug-reproduction/solidity-assembly-variable-reset/runs/8231871860?check_suite_focus=true

I add the example with the extra log1 and it passes

I didn't use the docker-compose. I'll take a look at it as well, thanks. I was trying to first investigate the contract.

wighawag · 2022-09-09T10:20:39Z

I updated the repo (https://github.com/bug-reproduction/solidity-assembly-variable-reset) to have hardhat test works too (works in the sense that the weird behavior is also shown)

pnpm i
pnpm hardhat test

wighawag · 2022-09-16T13:43:18Z

I updated the test and now specify specific gas limit and it passes

Probably the estimate_gas do not calculate enough for some reason. and when the log1 is added the estimate is correct for some reason

Should have thought about that, I guess we can close the issue unless you want to keep for the segmentation fault you find

r0qs · 2022-09-19T11:34:43Z

Hi @wighawag thank you very much for confirming the gas issue.

I will be closing the issue for now since your problem was solved, and it is unlikely that the compiler caused it. Still, I will further investigate your suspicion of the miscalculation of the gas estimation and the use of log1.

nikola-matic added the research label Sep 7, 2022

r0qs mentioned this issue Sep 7, 2022

Segmentation fault compiling a contract with huge data and via-IR enabled #13496

Closed

r0qs closed this as completed Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assembly clearing local variable #13494

Assembly clearing local variable #13494

wighawag commented Sep 7, 2022 •

edited

Loading

hrkrshnn commented Sep 7, 2022

ekpyron commented Sep 7, 2022

wighawag commented Sep 7, 2022 •

edited

Loading

r0qs commented Sep 7, 2022

wighawag commented Sep 7, 2022 •

edited

Loading

ekpyron commented Sep 7, 2022

wighawag commented Sep 7, 2022

r0qs commented Sep 7, 2022 •

edited

Loading

r0qs commented Sep 7, 2022 •

edited

Loading

wighawag commented Sep 9, 2022

wighawag commented Sep 16, 2022

r0qs commented Sep 19, 2022

Assembly clearing local variable #13494

Assembly clearing local variable #13494

Comments

wighawag commented Sep 7, 2022 • edited Loading

Description

Environment

Steps to Reproduce

hrkrshnn commented Sep 7, 2022

ekpyron commented Sep 7, 2022

wighawag commented Sep 7, 2022 • edited Loading

r0qs commented Sep 7, 2022

wighawag commented Sep 7, 2022 • edited Loading

ekpyron commented Sep 7, 2022

wighawag commented Sep 7, 2022

r0qs commented Sep 7, 2022 • edited Loading

r0qs commented Sep 7, 2022 • edited Loading

wighawag commented Sep 9, 2022

wighawag commented Sep 16, 2022

r0qs commented Sep 19, 2022

wighawag commented Sep 7, 2022 •

edited

Loading

wighawag commented Sep 7, 2022 •

edited

Loading

wighawag commented Sep 7, 2022 •

edited

Loading

r0qs commented Sep 7, 2022 •

edited

Loading

r0qs commented Sep 7, 2022 •

edited

Loading