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

ICE when loading the same file multiple times via import callback #15458

Open
cameel opened this issue Sep 25, 2024 · 1 comment
Open

ICE when loading the same file multiple times via import callback #15458

cameel opened this issue Sep 25, 2024 · 1 comment
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. should compile without error Error is reported even though it shouldn't. Source is fine.

Comments

@cameel
Copy link
Member

cameel commented Sep 25, 2024

Description

When the same path is specified under urls of multiple contracts in Standard JSON input, we get an ICE in the import callback.

Environment

  • Compiler version: 0.8.27

Steps to Reproduce

input.json:

{
    "language": "Solidity",
    "sources": {
        "A": {"urls": ["/tmp/test.sol"]},
        "B": {"urls": ["/tmp/test.sol"]}
    }
}

/tmp/test.sol

// SPDX-License-Identifier: MIT
pragma solidity *;

contract C {}
solc --standard-json input.json --pretty-json --json-indent 4

Output:

{
    "errors": [
        {
            "component": "general",
            "formattedMessage": "Cannot import url (\"/tmp/test.sol\"): Exception in read callback: /solidity/libsolidity/interface/FileReader.cpp(164): Throw in function solidity::frontend::ReadCallback::Result solidity::frontend::FileReader::readFile(const std::string&, const std::string&)\nDynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>\nstd::exception::what: Solidity assertion failed\n[solidity::util::tag_comment*] = Solidity assertion failed\n",
            "message": "Cannot import url (\"/tmp/test.sol\"): Exception in read callback: /solidity/libsolidity/interface/FileReader.cpp(164): Throw in function solidity::frontend::ReadCallback::Result solidity::frontend::FileReader::readFile(const std::string&, const std::string&)\nDynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>\nstd::exception::what: Solidity assertion failed\n[solidity::util::tag_comment*] = Solidity assertion failed\n",
            "severity": "error",
            "type": "IOError"
        }
    ],
    "sources": {
        "A": {
            "id": 0
        }
    }
}
@cameel cameel added bug 🐛 should compile without error Error is reported even though it shouldn't. Source is fine. low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. labels Sep 25, 2024
@cameel
Copy link
Member Author

cameel commented Sep 25, 2024

The immediate cause is that the callback has this weird flow, where the sources are not stored in a single place, but independently in both FileReader and CompilerStack. We initially fill the FileReader, then pass the sources into CompilerStack, which may add new ones loaded by the callback, but the reader still exists and updates its m_sourceCodes map:

solAssert(m_sourceCodes.count(_sourceUnitName) == 0, "");
m_sourceCodes[_sourceUnitName] = contents;
return ReadCallback::Result{true, contents};

We should get rid of that. It would be best to separate the storage of the sources from the callback. The callback should know its settings but be stateless otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. should compile without error Error is reported even though it shouldn't. Source is fine.
Projects
None yet
Development

No branches or pull requests

1 participant