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

Compiler stack: Protect loadGeneratedIR from contracts that cannot be deployed #15505

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

clonker
Copy link
Member

@clonker clonker commented Oct 11, 2024

If a contract is not deployable (ie interface / abstract), no yulIR will be generated. This causes the object parser to error out in loadGeneratedIR, as it expects object {} as a bare minimum, but gets an empty string instead.

@clonker clonker requested a review from cameel October 11, 2024 13:10
@r0qs
Copy link
Member

r0qs commented Oct 11, 2024

I would also add a test case like:

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

abstract contract C {
	function f() public virtual returns (string memory);
}

and

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

interface I {
	function f() external returns (string memory);
}

Could be similar to our current ast ir command-line test (https://github.com/ethereum/solidity/blob/e8b5ca83f37b6a126db6a0a1dcfa00decff63647/test/cmdlineTests/ast_ir/args). And also one, or more, for standard Json output (https://github.com/ethereum/solidity/blob/e8b5ca83f37b6a126db6a0a1dcfa00decff63647/test/cmdlineTests/standard_ir_ast_requested/input.json):

{
	"language": "Solidity",
	"sources":
	{
		"C":
		{
			"content": "// SPDX-License-Identifier: GPL-3.0\npragma solidity >=0.0; abstract contract C {}"
		}
	},
	"settings": {
		"optimizer": {
			"enabled": true,
			"details": {
				"yul": true
			}
		},
		"viaIR": true,
		"outputSelection": {
			"*": {
				"*": ["irAst", "irOptimizedAst", "yulCFGJson"]
			}
		}
	}

}

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Good catch. Looks like I unwittingly introduced this bug in the last release via #15451.

The fix is not wrong, but I'd do it differently in CompilerStack to make this kind of mistake harder to make in the future.

libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved
test/cmdlineTests/ast_ir_abstract_contract/input.sol Outdated Show resolved Hide resolved
libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved
r0qs
r0qs previously approved these changes Oct 14, 2024
Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

I have only one suggestion. But it looks good to me anyway :)

r0qs
r0qs previously approved these changes Oct 14, 2024
@ekpyron
Copy link
Member

ekpyron commented Oct 14, 2024

This should probably also get a bugfix changelog entry. A bit annoying that this is in the release now, but I guess people rarely use these json artifacts anyways - might be a good indicator for how much they're used in the wild actually (since if they're used, people will quickly run into this I guess)

Changelog.md Outdated Show resolved Hide resolved
@clonker clonker force-pushed the protect_load_generated_ir branch 2 times, most recently from 3548b2d to 1f20ccd Compare October 14, 2024 16:53
Changelog.md Outdated Show resolved Hide resolved
libsolidity/interface/StandardCompiler.cpp Outdated Show resolved Hide resolved
solc/CommandLineInterface.cpp Outdated Show resolved Hide resolved
test/cmdlineTests/ast_ir_undeployable_contract/args Outdated Show resolved Hide resolved
test/libsolidity/MemoryGuardTest.cpp Show resolved Hide resolved
libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved
test/cmdlineTests/ast_ir_undeployable_contract/args Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
@cameel cameel added the 🟡 PR review label label Oct 16, 2024
@clonker clonker merged commit 3994386 into develop Oct 16, 2024
73 checks passed
@clonker clonker deleted the protect_load_generated_ir branch October 16, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟡 PR review label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants