From 21360e3c1a3f1cae441d268f0ccaeb29e0490808 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 16 Mar 2023 15:11:04 -0500 Subject: [PATCH] fix: Prevent calling contract functions from outside the contract (#980) * Prevent calling contract functions from outside the contract * Fix tests * Improve error messages for imports; give only the part of the path that could not be found --- crates/iter-extended/src/lib.rs | 21 ++++ .../tests/test_data/contracts/src/main.nr | 2 +- .../src/hir/def_collector/dc_crate.rs | 14 +-- .../src/hir/def_collector/errors.rs | 20 +-- .../src/hir/resolution/errors.rs | 21 +--- .../src/hir/resolution/import.rs | 116 ++++++++++++------ .../src/hir/resolution/path_resolver.rs | 20 +-- .../src/hir/resolution/resolver.rs | 22 ++-- .../noirc_frontend/src/hir/type_check/mod.rs | 10 +- 9 files changed, 141 insertions(+), 105 deletions(-) diff --git a/crates/iter-extended/src/lib.rs b/crates/iter-extended/src/lib.rs index 00824d1ee53..a022ad00b9e 100644 --- a/crates/iter-extended/src/lib.rs +++ b/crates/iter-extended/src/lib.rs @@ -41,3 +41,24 @@ where { iterable.into_iter().map(f).collect() } + +/// Given an iterator over a Result, filter out the Ok values from the Err values +/// and return both in separate Vecs. Unlike other collect-like functions over Results, +/// this function will always consume the entire iterator. +pub fn partition_results(iterable: It, mut f: F) -> (Vec, Vec) +where + It: IntoIterator, + F: FnMut(It::Item) -> Result, +{ + let mut oks = vec![]; + let mut errors = vec![]; + + for elem in iterable { + match f(elem) { + Ok(ok) => oks.push(ok), + Err(error) => errors.push(error), + } + } + + (oks, errors) +} diff --git a/crates/nargo/tests/test_data/contracts/src/main.nr b/crates/nargo/tests/test_data/contracts/src/main.nr index 092439ad93a..98233a34922 100644 --- a/crates/nargo/tests/test_data/contracts/src/main.nr +++ b/crates/nargo/tests/test_data/contracts/src/main.nr @@ -1,5 +1,5 @@ fn main(x : Field, y : pub Field) { - constrain Foo::double(x) == Foo::triple(y); + constrain x * 2 == y * 3; } contract Foo { diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index ef9986796bc..1133b4c052f 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -122,16 +122,16 @@ impl DefCollector { context.def_maps.insert(crate_id, def_collector.def_map); // Resolve unresolved imports collected from the crate - let (unresolved, resolved) = + let (resolved, unresolved_imports) = resolve_imports(crate_id, def_collector.collected_imports, &context.def_maps); let current_def_map = context.def_maps.get(&crate_id).unwrap(); - for unresolved_import in unresolved.into_iter() { - // File if that the import was declared - let file_id = current_def_map.modules[unresolved_import.module_id.0].origin.file_id(); - let error = DefCollectorErrorKind::UnresolvedImport { import: unresolved_import }; - errors.push(error.into_file_diagnostic(file_id)); - } + + errors.extend(vecmap(unresolved_imports, |(error, module_id)| { + let file_id = current_def_map.modules[module_id.0].origin.file_id(); + let error = DefCollectorErrorKind::PathResolutionError(error); + error.into_file_diagnostic(file_id) + })); // Populate module namespaces according to the imports used let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); diff --git a/crates/noirc_frontend/src/hir/def_collector/errors.rs b/crates/noirc_frontend/src/hir/def_collector/errors.rs index 6ee97061005..d18df58d64d 100644 --- a/crates/noirc_frontend/src/hir/def_collector/errors.rs +++ b/crates/noirc_frontend/src/hir/def_collector/errors.rs @@ -1,4 +1,5 @@ -use crate::{hir::resolution::import::ImportDirective, Ident}; +use crate::hir::resolution::import::PathResolutionError; +use crate::Ident; use noirc_errors::CustomDiagnostic as Diagnostic; use noirc_errors::FileDiagnostic; @@ -17,8 +18,8 @@ pub enum DefCollectorErrorKind { DuplicateGlobal { first_def: Ident, second_def: Ident }, #[error("unresolved import")] UnresolvedModuleDecl { mod_name: Ident }, - #[error("unresolved import")] - UnresolvedImport { import: ImportDirective }, + #[error("path resolution error")] + PathResolutionError(PathResolutionError), #[error("Non-struct type used in impl")] NonStructTypeInImpl { span: Span }, } @@ -94,18 +95,7 @@ impl From for Diagnostic { span, ) } - DefCollectorErrorKind::UnresolvedImport { import } => { - let mut span = import.path.span(); - if let Some(alias) = &import.alias { - span = span.merge(alias.0.span()) - } - - Diagnostic::simple_error( - format!("could not resolve import {}", &import.path.as_string()), - String::new(), - span, - ) - } + DefCollectorErrorKind::PathResolutionError(error) => error.into(), DefCollectorErrorKind::NonStructTypeInImpl { span } => Diagnostic::simple_error( "Non-struct type used in impl".into(), "Only struct types may have implementation methods".into(), diff --git a/crates/noirc_frontend/src/hir/resolution/errors.rs b/crates/noirc_frontend/src/hir/resolution/errors.rs index 330a18072c4..6b52ab03b46 100644 --- a/crates/noirc_frontend/src/hir/resolution/errors.rs +++ b/crates/noirc_frontend/src/hir/resolution/errors.rs @@ -4,6 +4,8 @@ use thiserror::Error; use crate::{Ident, Shared, StructType, Type}; +use super::import::PathResolutionError; + #[derive(Error, Debug, Clone, PartialEq, Eq)] pub enum ResolverError { #[error("Duplicate definition")] @@ -15,7 +17,7 @@ pub enum ResolverError { #[error("path is not an identifier")] PathIsNotIdent { span: Span }, #[error("could not resolve path")] - PathUnresolved { span: Span, name: String, segment: Ident }, + PathResolutionError(PathResolutionError), #[error("Expected")] Expected { span: Span, expected: String, got: String }, #[error("Duplicate field in constructor")] @@ -99,22 +101,7 @@ impl From for Diagnostic { String::new(), span, ), - ResolverError::PathUnresolved { span, name, segment } => { - let mut diag = Diagnostic::simple_error( - format!("could not resolve path '{name}'"), - String::new(), - span, - ); - // XXX: When the secondary and primary labels have spans that - // overlap, you cannot differentiate between them. - // This error is an example of this. - diag.add_secondary( - format!("could not resolve `{}` in path", &segment.0.contents), - segment.0.span(), - ); - - diag - } + ResolverError::PathResolutionError(error) => error.into(), ResolverError::Expected { span, expected, got } => Diagnostic::simple_error( format!("expected {expected} got {got}"), String::new(), diff --git a/crates/noirc_frontend/src/hir/resolution/import.rs b/crates/noirc_frontend/src/hir/resolution/import.rs index aa2fd37e64a..ce7d1503fb7 100644 --- a/crates/noirc_frontend/src/hir/resolution/import.rs +++ b/crates/noirc_frontend/src/hir/resolution/import.rs @@ -1,21 +1,27 @@ +use iter_extended::partition_results; +use noirc_errors::CustomDiagnostic; + use crate::graph::CrateId; use std::collections::HashMap; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs}; use crate::{Ident, Path, PathKind}; -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ImportDirective { pub module_id: LocalModuleId, pub path: Path, pub alias: Option, } -#[derive(Debug)] -pub enum PathResolution { - Resolved(PerNs), +pub type PathResolution = Result; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PathResolutionError { Unresolved(Ident), + ExternalContractUsed(Ident), } + #[derive(Debug)] pub struct ResolvedImport { // name of the namespace, either last path segment or an alias @@ -26,59 +32,78 @@ pub struct ResolvedImport { pub module_scope: LocalModuleId, } +impl From for CustomDiagnostic { + fn from(error: PathResolutionError) -> Self { + match error { + PathResolutionError::Unresolved(ident) => CustomDiagnostic::simple_error( + format!("Could not resolve '{ident}' in path"), + String::new(), + ident.span(), + ), + PathResolutionError::ExternalContractUsed(ident) => CustomDiagnostic::simple_error( + format!("Contract variable '{ident}' referenced from outside the contract"), + "Contracts may only be referenced from within a contract".to_string(), + ident.span(), + ), + } + } +} + pub fn resolve_imports( crate_id: CrateId, imports_to_resolve: Vec, def_maps: &HashMap, -) -> (Vec, Vec) { - let num_imports = imports_to_resolve.len(); +) -> (Vec, Vec<(PathResolutionError, LocalModuleId)>) { let def_map = &def_maps[&crate_id]; - let mut unresolved: Vec = Vec::new(); - let mut resolved: Vec = Vec::new(); - for import_directive in imports_to_resolve { - let defs = resolve_path_to_ns(&import_directive, def_map, def_maps); - - // Once we have the Option - // resolve name and push into appropriate vector - match defs { - PathResolution::Unresolved(_) => { - unresolved.push(import_directive); - } - PathResolution::Resolved(resolved_namespace) => { - let name = resolve_path_name(&import_directive); - let res = ResolvedImport { - name, - resolved_namespace, - module_scope: import_directive.module_id, - }; - resolved.push(res); - } - }; - } + partition_results(imports_to_resolve, |import_directive| { + let allow_contracts = + allow_referencing_contracts(def_maps, crate_id, import_directive.module_id); - assert!(unresolved.len() + resolved.len() == num_imports); + let module_scope = import_directive.module_id; + let resolved_namespace = + resolve_path_to_ns(&import_directive, def_map, def_maps, allow_contracts) + .map_err(|error| (error, module_scope))?; - (unresolved, resolved) + let name = resolve_path_name(&import_directive); + Ok(ResolvedImport { name, resolved_namespace, module_scope }) + }) +} + +pub(super) fn allow_referencing_contracts( + def_maps: &HashMap, + krate: CrateId, + module: LocalModuleId, +) -> bool { + def_maps[&krate].modules()[module.0].is_contract } pub fn resolve_path_to_ns( import_directive: &ImportDirective, def_map: &CrateDefMap, def_maps: &HashMap, + allow_contracts: bool, ) -> PathResolution { let import_path = &import_directive.path.segments; match import_directive.path.kind { crate::ast::PathKind::Crate => { // Resolve from the root of the crate - resolve_path_from_crate_root(def_map, import_path, def_maps) + resolve_path_from_crate_root(def_map, import_path, def_maps, allow_contracts) + } + crate::ast::PathKind::Dep => { + resolve_external_dep(def_map, import_directive, def_maps, allow_contracts) } - crate::ast::PathKind::Dep => resolve_external_dep(def_map, import_directive, def_maps), crate::ast::PathKind::Plain => { // Plain paths are only used to import children modules. It's possible to allow import of external deps, but maybe this distinction is better? // In Rust they can also point to external Dependencies, if no children can be found with the specified name - resolve_name_in_module(def_map, import_path, import_directive.module_id, def_maps) + resolve_name_in_module( + def_map, + import_path, + import_directive.module_id, + def_maps, + allow_contracts, + ) } } } @@ -87,8 +112,9 @@ fn resolve_path_from_crate_root( def_map: &CrateDefMap, import_path: &[Ident], def_maps: &HashMap, + allow_contracts: bool, ) -> PathResolution { - resolve_name_in_module(def_map, import_path, def_map.root, def_maps) + resolve_name_in_module(def_map, import_path, def_map.root, def_maps, allow_contracts) } fn resolve_name_in_module( @@ -96,6 +122,7 @@ fn resolve_name_in_module( import_path: &[Ident], starting_mod: LocalModuleId, def_maps: &HashMap, + allow_contracts: bool, ) -> PathResolution { let mut current_mod = &def_map.modules[starting_mod.0]; @@ -103,19 +130,19 @@ fn resolve_name_in_module( // In that case, early return if import_path.is_empty() { let mod_id = ModuleId { krate: def_map.krate, local_id: starting_mod }; - return PathResolution::Resolved(PerNs::types(mod_id.into())); + return Ok(PerNs::types(mod_id.into())); } let mut import_path = import_path.iter(); let first_segment = import_path.next().expect("ice: could not fetch first segment"); let mut current_ns = current_mod.scope.find_name(first_segment); if current_ns.is_none() { - return PathResolution::Unresolved(first_segment.clone()); + return Err(PathResolutionError::Unresolved(first_segment.clone())); } for segment in import_path { let typ = match current_ns.take_types() { - None => return PathResolution::Unresolved(segment.clone()), + None => return Err(PathResolutionError::Unresolved(segment.clone())), Some(typ) => typ, }; @@ -127,16 +154,24 @@ fn resolve_name_in_module( ModuleDefId::TypeId(id) => id.0, ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), }; + current_mod = &def_maps[&new_module_id.krate].modules[new_module_id.local_id.0]; + // Check if namespace let found_ns = current_mod.scope.find_name(segment); if found_ns.is_none() { - return PathResolution::Unresolved(segment.clone()); + return Err(PathResolutionError::Unresolved(segment.clone())); } + + // Check if it is a contract and we're calling from a non-contract context + if current_mod.is_contract && !allow_contracts { + return Err(PathResolutionError::ExternalContractUsed(segment.clone())); + } + current_ns = found_ns } - PathResolution::Resolved(current_ns) + Ok(current_ns) } fn resolve_path_name(import_directive: &ImportDirective) -> Ident { @@ -150,6 +185,7 @@ fn resolve_external_dep( current_def_map: &CrateDefMap, directive: &ImportDirective, def_maps: &HashMap, + allow_contracts: bool, ) -> PathResolution { // Use extern_prelude to get the dep // @@ -171,5 +207,5 @@ fn resolve_external_dep( let dep_def_map = def_maps.get(&dep_module.krate).unwrap(); - resolve_path_to_ns(&dep_directive, dep_def_map, def_maps) + resolve_path_to_ns(&dep_directive, dep_def_map, def_maps, allow_contracts) } diff --git a/crates/noirc_frontend/src/hir/resolution/path_resolver.rs b/crates/noirc_frontend/src/hir/resolution/path_resolver.rs index 74edbb180a0..57cf4c709c7 100644 --- a/crates/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,5 +1,7 @@ -use super::import::{resolve_path_to_ns, ImportDirective, PathResolution}; -use crate::{Ident, Path}; +use super::import::{ + allow_referencing_contracts, resolve_path_to_ns, ImportDirective, PathResolutionError, +}; +use crate::Path; use std::collections::HashMap; use crate::graph::CrateId; @@ -11,7 +13,7 @@ pub trait PathResolver { &self, def_maps: &HashMap, path: Path, - ) -> Result; + ) -> Result; fn local_module_id(&self) -> LocalModuleId; } @@ -32,7 +34,7 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &HashMap, path: Path, - ) -> Result { + ) -> Result { resolve_path(def_maps, self.module_id, path) } @@ -47,16 +49,14 @@ pub fn resolve_path( def_maps: &HashMap, module_id: ModuleId, path: Path, -) -> Result { +) -> Result { // lets package up the path into an ImportDirective and resolve it using that let import = ImportDirective { module_id: module_id.local_id, path, alias: None }; + let allow_referencing_contracts = + allow_referencing_contracts(def_maps, module_id.krate, module_id.local_id); let def_map = &def_maps[&module_id.krate]; - let path_res = resolve_path_to_ns(&import, def_map, def_maps); - let ns = match path_res { - PathResolution::Unresolved(seg) => return Err(seg), - PathResolution::Resolved(ns) => ns, - }; + let ns = resolve_path_to_ns(&import, def_map, def_maps, allow_referencing_contracts)?; let function = ns.values.map(|(id, _)| id); let id = function.or_else(|| ns.types.map(|(id, _)| id)); diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 5660206b947..3fe8dadf38e 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -1048,11 +1048,7 @@ impl<'a> Resolver<'a> { } fn resolve_path(&mut self, path: Path) -> Result { - let span = path.span(); - let name = path.as_string(); - self.path_resolver - .resolve(self.def_maps, path) - .map_err(|segment| ResolverError::PathUnresolved { name, span, segment }) + self.path_resolver.resolve(self.def_maps, path).map_err(ResolverError::PathResolutionError) } fn resolve_block(&mut self, block_expr: BlockExpression) -> HirExpression { @@ -1212,7 +1208,8 @@ mod test { use fm::FileId; use iter_extended::vecmap; - use crate::{hir::resolution::errors::ResolverError, Ident}; + use crate::hir::resolution::errors::ResolverError; + use crate::hir::resolution::import::PathResolutionError; use crate::graph::CrateId; use crate::hir_def::function::HirFunction; @@ -1337,7 +1334,7 @@ mod test { assert_eq!(errors.len(), 1); let err = errors.pop().unwrap(); - path_unresolved_error(err, "some::path::to::a::func"); + path_unresolved_error(err, "func"); } #[test] @@ -1377,11 +1374,12 @@ mod test { ResolverError::VariableNotDeclared { name, .. } => { assert_eq!(name, "a"); } - ResolverError::PathUnresolved { .. } => path_unresolved_error(err, "foo::bar"), + ResolverError::PathResolutionError(_) => path_unresolved_error(err, "bar"), _ => unimplemented!(), }; } } + #[test] fn resolve_prefix_expr() { let src = r#" @@ -1424,8 +1422,8 @@ mod test { fn path_unresolved_error(err: ResolverError, expected_unresolved_path: &str) { match err { - ResolverError::PathUnresolved { span: _, name, segment: _ } => { - assert_eq!(name, expected_unresolved_path) + ResolverError::PathResolutionError(PathResolutionError::Unresolved(name)) => { + assert_eq!(name.to_string(), expected_unresolved_path) } _ => unimplemented!("expected an unresolved path"), } @@ -1438,11 +1436,11 @@ mod test { &self, _def_maps: &HashMap, path: Path, - ) -> Result { + ) -> Result { // Not here that foo::bar and hello::foo::bar would fetch the same thing let name = path.segments.last().unwrap(); let mod_def = self.0.get(&name.0.contents).cloned(); - mod_def.ok_or_else(|| name.clone()) + mod_def.ok_or_else(move || PathResolutionError::Unresolved(name.clone())) } fn local_module_id(&self) -> LocalModuleId { diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index d0395a6797f..08fc57f48e7 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -74,6 +74,8 @@ mod test { use iter_extended::vecmap; use noirc_errors::{Location, Span}; + use crate::graph::CrateId; + use crate::hir::resolution::import::PathResolutionError; use crate::hir_def::expr::HirIdent; use crate::hir_def::stmt::HirLetStatement; use crate::hir_def::stmt::HirPattern::Identifier; @@ -85,7 +87,6 @@ mod test { }; use crate::node_interner::{DefinitionKind, FuncId, NodeInterner}; use crate::BinaryOpKind; - use crate::{graph::CrateId, Ident}; use crate::{ hir::{ def_map::{CrateDefMap, LocalModuleId, ModuleDefId}, @@ -233,10 +234,13 @@ mod test { &self, _def_maps: &HashMap, path: Path, - ) -> Result { + ) -> Result { // Not here that foo::bar and hello::foo::bar would fetch the same thing let name = path.segments.last().unwrap(); - self.0.get(&name.0.contents).cloned().ok_or_else(|| name.clone()) + self.0 + .get(&name.0.contents) + .cloned() + .ok_or_else(move || PathResolutionError::Unresolved(name.clone())) } fn local_module_id(&self) -> LocalModuleId {