From f909555022152188338e70a1409fd4f87bea6237 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20=C5=9Aliwak?= Date: Thu, 26 Aug 2021 18:35:25 +0200 Subject: [PATCH] Treat ambiguous imports as errors --- docs/path-resolution.rst | 9 ++++ libsolidity/interface/FileReader.cpp | 33 +++++++++++---- test/solc/CommandLineInterface.cpp | 44 ++++++++++++++++++++ test/solc/CommandLineInterfaceAllowPaths.cpp | 6 ++- 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/docs/path-resolution.rst b/docs/path-resolution.rst index f1a246788..40a727068 100644 --- a/docs/path-resolution.rst +++ b/docs/path-resolution.rst @@ -339,6 +339,15 @@ of the compiler. Include paths cannot have empty values and must be used together with a non-empty base path. +.. note:: + + Include paths and base path can overlap as long as it does not make import resolution ambiguous. + For example, you can specify a directory inside base path as an include directory or have an + include directory that is a subdirectory of another include directory. + The compiler will only issue an error if the source unit name passed to the Host Filesystem + Loader represents an existing path when combined with multiple include paths or an include path + and base path. + .. _cli-path-normalization-and-stripping: CLI Path Normalization and Stripping diff --git a/libsolidity/interface/FileReader.cpp b/libsolidity/interface/FileReader.cpp index 4d60b8083..2a1859d21 100644 --- a/libsolidity/interface/FileReader.cpp +++ b/libsolidity/interface/FileReader.cpp @@ -21,8 +21,10 @@ #include #include +#include #include +#include #include @@ -32,6 +34,7 @@ using solidity::frontend::ReadCallback; using solidity::langutil::InternalCompilerError; using solidity::util::errinfo_comment; using solidity::util::readFileAsString; +using solidity::util::joinHumanReadable; using std::map; using std::reference_wrapper; using std::string; @@ -109,24 +112,38 @@ ReadCallback::Result FileReader::readFile(string const& _kind, string const& _so if (strippedSourceUnitName.find("file://") == 0) strippedSourceUnitName.erase(0, 7); + vector candidates; vector> prefixes = {m_basePath}; prefixes += (m_includePaths | ranges::to>>); - boost::filesystem::path canonicalPath; for (auto const& prefix: prefixes) { - canonicalPath = normalizeCLIPathForVFS(prefix / strippedSourceUnitName, SymlinkResolution::Enabled); + boost::filesystem::path canonicalPath = normalizeCLIPathForVFS(prefix / strippedSourceUnitName, SymlinkResolution::Enabled); if (boost::filesystem::exists(canonicalPath)) - break; + candidates.push_back(std::move(canonicalPath)); } + auto pathToQuotedString = [](boost::filesystem::path const& _path){ return "\"" + _path.string() + "\""; }; + + if (candidates.empty()) + return ReadCallback::Result{false, "File not found."}; + + if (candidates.size() >= 2) + return ReadCallback::Result{ + false, + "Ambiguous import. " + "Multiple matching files found inside base path and/or include paths: " + + joinHumanReadable(candidates | ranges::views::transform(pathToQuotedString), ", ") + + "." + }; + FileSystemPathSet extraAllowedPaths = {m_basePath.empty() ? "." : m_basePath}; extraAllowedPaths += m_includePaths; bool isAllowed = false; for (boost::filesystem::path const& allowedDir: m_allowedDirectories + extraAllowedPaths) - if (isPathPrefix(normalizeCLIPathForVFS(allowedDir, SymlinkResolution::Enabled), canonicalPath)) + if (isPathPrefix(normalizeCLIPathForVFS(allowedDir, SymlinkResolution::Enabled), candidates[0])) { isAllowed = true; break; @@ -135,14 +152,12 @@ ReadCallback::Result FileReader::readFile(string const& _kind, string const& _so if (!isAllowed) return ReadCallback::Result{false, "File outside of allowed directories."}; - if (!boost::filesystem::exists(canonicalPath)) - return ReadCallback::Result{false, "File not found."}; - - if (!boost::filesystem::is_regular_file(canonicalPath)) + if (!boost::filesystem::is_regular_file(candidates[0])) return ReadCallback::Result{false, "Not a valid file."}; // NOTE: we ignore the FileNotFound exception as we manually check above - auto contents = readFileAsString(canonicalPath); + auto contents = readFileAsString(candidates[0]); + solAssert(m_sourceCodes.count(_sourceUnitName) == 0, ""); m_sourceCodes[_sourceUnitName] = contents; return ReadCallback::Result{true, contents}; } diff --git a/test/solc/CommandLineInterface.cpp b/test/solc/CommandLineInterface.cpp index 7d7470db5..126a1cd09 100644 --- a/test/solc/CommandLineInterface.cpp +++ b/test/solc/CommandLineInterface.cpp @@ -1247,6 +1247,50 @@ BOOST_AUTO_TEST_CASE(cli_include_paths_should_allow_duplicate_paths) BOOST_TEST(result.reader.basePath() == expectedWorkDir / "dir1/"); } +BOOST_AUTO_TEST_CASE(cli_include_paths_ambiguous_import) +{ + TemporaryDirectory tempDir({"base/", "include/"}, TEST_CASE_NAME); + TemporaryWorkingDirectory tempWorkDir(tempDir); + + string const preamble = + "// SPDX-License-Identifier: GPL-3.0\n" + "pragma solidity >=0.0;\n"; + string const mainContractSource = preamble + + // Ambiguous: both base/contract.sol and include/contract.sol match the import. + "import \"contract.sol\";"; + + createFilesWithParentDirs({"base/contract.sol", "include/contract.sol"}, preamble); + + boost::filesystem::path expectedWorkDir = "/" / boost::filesystem::canonical(tempDir).relative_path(); + + vector commandLine = { + "solc", + "--no-color", + "--base-path=base/", + "--include-path=include/", + "-", + }; + + string expectedMessage = + "Error: Source \"contract.sol\" not found: Ambiguous import. " + "Multiple matching files found inside base path and/or include paths: \"" + + (expectedWorkDir / "base/contract.sol").generic_string() + "\", \"" + + (expectedWorkDir / "include/contract.sol").generic_string() + "\".\n" + " --> :3:1:\n" + " |\n" + "3 | import \"contract.sol\";\n" + " | ^^^^^^^^^^^^^^^^^^^^^^\n\n"; + + OptionsReaderAndMessages result = parseCommandLineAndReadInputFiles( + commandLine, + mainContractSource, + true /* _processInput */ + ); + + BOOST_TEST(result.stderrContent == expectedMessage); + BOOST_REQUIRE(!result.success); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace solidity::frontend::test diff --git a/test/solc/CommandLineInterfaceAllowPaths.cpp b/test/solc/CommandLineInterfaceAllowPaths.cpp index d48471acf..215085699 100644 --- a/test/solc/CommandLineInterfaceAllowPaths.cpp +++ b/test/solc/CommandLineInterfaceAllowPaths.cpp @@ -344,10 +344,14 @@ BOOST_FIXTURE_TEST_CASE(allow_path_should_work_with_various_import_forms, AllowP BOOST_TEST(checkImport("import 'a/../../code/a/../a/b/c.sol'", {"--allow-paths", "../code/a/b/c.sol"})); BOOST_TEST(checkImport("import 'a/../../code/a///b/c.sol'", {"--allow-paths", "../code/a/b/c.sol"})); - // UNC paths in imports +#if !defined(_WIN32) + // UNC paths in imports. + // Unfortunately can't test it on Windows without having an existing UNC path. On Linux we can + // at least rely on the fact that `//` works like `/`. string uncImportPath = "/" + m_portablePrefix + "/a/b/c.sol"; soltestAssert(FileReader::isUNCPath(uncImportPath), ""); BOOST_TEST(checkImport("import '" + uncImportPath + "'", {"--allow-paths", "../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); +#endif } BOOST_FIXTURE_TEST_CASE(allow_path_automatic_whitelisting_input_files, AllowPathsFixture)