From 34f352ff2b693f0f900eebc3af73d08b59fbe0f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rain=20=E2=81=A3?= Date: Tue, 18 Jun 2019 12:26:57 -0700 Subject: [PATCH] [vm] move signature checking implementation to bytecode verifier This is a more natural place to have these checks. Talked with @shaz about this. --- language/bytecode_verifier/src/lib.rs | 2 + language/bytecode_verifier/src/signature.rs | 91 +++++++++++++++++-- .../src/unit_tests.rs} | 0 .../src/unit_tests}/signature_tests.rs | 15 +-- language/vm/src/checks.rs | 2 - language/vm/src/checks/signature.rs | 91 ------------------- language/vm/src/unit_tests/mod.rs | 1 - 7 files changed, 93 insertions(+), 109 deletions(-) rename language/{vm/src/unit_tests/checks.rs => bytecode_verifier/src/unit_tests.rs} (100%) rename language/{vm/src/unit_tests/checks => bytecode_verifier/src/unit_tests}/signature_tests.rs (77%) delete mode 100644 language/vm/src/checks/signature.rs diff --git a/language/bytecode_verifier/src/lib.rs b/language/bytecode_verifier/src/lib.rs index e477c86a9960..a9460aee1147 100644 --- a/language/bytecode_verifier/src/lib.rs +++ b/language/bytecode_verifier/src/lib.rs @@ -18,6 +18,8 @@ pub mod resources; pub mod signature; pub mod stack_usage_verifier; pub mod struct_defs; +#[cfg(test)] +mod unit_tests; pub mod verifier; pub use check_duplication::DuplicationChecker; diff --git a/language/bytecode_verifier/src/signature.rs b/language/bytecode_verifier/src/signature.rs index 7e7a25cb8722..aeb0505360ab 100644 --- a/language/bytecode_verifier/src/signature.rs +++ b/language/bytecode_verifier/src/signature.rs @@ -5,8 +5,14 @@ //! parameters, locals, and fields of structs are well-formed. References can only occur at the //! top-level in all tokens. Additionally, references cannot occur at all in field types. use vm::{ - checks::SignatureCheck, errors::VerificationError, file_format::CompiledModule, - views::ModuleView, IndexKind, + access::ModuleAccess, + errors::{VMStaticViolation, VerificationError}, + file_format::{CompiledModule, SignatureToken}, + views::{ + FieldDefinitionView, FunctionSignatureView, LocalsSignatureView, ModuleView, + TypeSignatureView, ViewInternals, + }, + IndexKind, SignatureTokenKind, }; pub struct SignatureChecker<'a> { @@ -41,12 +47,11 @@ impl<'a> SignatureChecker<'a> { .fields() .enumerate() .filter_map(move |(idx, view)| { - view.check_signature_refs() - .map(move |err| VerificationError { - kind: IndexKind::FieldDefinition, - idx, - err, - }) + check_signature_refs(&view).map(move |err| VerificationError { + kind: IndexKind::FieldDefinition, + idx, + err, + }) }) .collect(); errors.push(signature_ref_errors); @@ -70,3 +75,73 @@ impl<'a> SignatureChecker<'a> { .collect() } } + +trait SignatureCheck { + fn check_signatures(&self) -> Vec; +} + +impl<'a, T: ModuleAccess> SignatureCheck for FunctionSignatureView<'a, T> { + fn check_signatures(&self) -> Vec { + self.return_tokens() + .filter_map(|token| check_structure(token.as_inner())) + .chain( + self.arg_tokens() + .filter_map(|token| check_structure(token.as_inner())), + ) + .collect() + } +} + +impl<'a, T: ModuleAccess> SignatureCheck for TypeSignatureView<'a, T> { + fn check_signatures(&self) -> Vec { + check_structure(self.token().as_inner()) + .into_iter() + .collect() + } +} + +impl<'a, T: ModuleAccess> SignatureCheck for LocalsSignatureView<'a, T> { + fn check_signatures(&self) -> Vec { + self.tokens() + .filter_map(|token| check_structure(token.as_inner())) + .collect() + } +} + +/// Field definitions have additional constraints on signatures -- field signatures cannot be +/// references or mutable references. +pub(crate) fn check_signature_refs( + view: &FieldDefinitionView<'_, CompiledModule>, +) -> Option { + let type_signature = view.type_signature(); + let token = type_signature.token(); + let kind = token.kind(); + match kind { + SignatureTokenKind::Reference | SignatureTokenKind::MutableReference => Some( + VMStaticViolation::InvalidFieldDefReference(token.as_inner().clone(), kind), + ), + SignatureTokenKind::Value => None, + } +} + +/// Check that this token is structurally correct. +/// In particular, check that the token has a reference only at the top level. +pub(crate) fn check_structure(token: &SignatureToken) -> Option { + use SignatureToken::*; + + let inner_token_opt = match token { + Reference(token) => Some(token), + MutableReference(token) => Some(token), + Bool | U64 | String | ByteArray | Address | Struct(_) => None, + }; + if let Some(inner_token) = inner_token_opt { + if inner_token.is_reference() { + return Some(VMStaticViolation::InvalidSignatureToken( + token.clone(), + token.kind(), + inner_token.kind(), + )); + } + } + None +} diff --git a/language/vm/src/unit_tests/checks.rs b/language/bytecode_verifier/src/unit_tests.rs similarity index 100% rename from language/vm/src/unit_tests/checks.rs rename to language/bytecode_verifier/src/unit_tests.rs diff --git a/language/vm/src/unit_tests/checks/signature_tests.rs b/language/bytecode_verifier/src/unit_tests/signature_tests.rs similarity index 77% rename from language/vm/src/unit_tests/checks/signature_tests.rs rename to language/bytecode_verifier/src/unit_tests/signature_tests.rs index 13532deb0fc8..aecbf66fc7b9 100644 --- a/language/vm/src/unit_tests/checks/signature_tests.rs +++ b/language/bytecode_verifier/src/unit_tests/signature_tests.rs @@ -1,7 +1,8 @@ // Copyright (c) The Libra Core Contributors // SPDX-License-Identifier: Apache-2.0 -use crate::{ +use crate::signature::check_structure; +use vm::{ errors::VMStaticViolation, file_format::{SignatureToken, StructHandleIndex}, SignatureTokenKind, @@ -11,18 +12,18 @@ use crate::{ fn test_sig_token_structure() { // Valid cases. let bool_token = SignatureToken::Bool; - assert_eq!(bool_token.check_structure(), None); + assert_eq!(check_structure(&bool_token), None); let struct_token = SignatureToken::Struct(StructHandleIndex::new(0)); - assert_eq!(struct_token.check_structure(), None); + assert_eq!(check_structure(&struct_token), None); let ref_token = SignatureToken::Reference(Box::new(struct_token.clone())); - assert_eq!(ref_token.check_structure(), None); + assert_eq!(check_structure(&ref_token), None); let mut_ref_token = SignatureToken::MutableReference(Box::new(struct_token.clone())); - assert_eq!(mut_ref_token.check_structure(), None); + assert_eq!(check_structure(&mut_ref_token), None); // Invalid cases. let ref_ref_token = SignatureToken::Reference(Box::new(ref_token.clone())); assert_eq!( - ref_ref_token.check_structure(), + check_structure(&ref_ref_token), Some(VMStaticViolation::InvalidSignatureToken( ref_ref_token.clone(), SignatureTokenKind::Reference, @@ -31,7 +32,7 @@ fn test_sig_token_structure() { ); let ref_mut_ref_token = SignatureToken::Reference(Box::new(mut_ref_token.clone())); assert_eq!( - ref_mut_ref_token.check_structure(), + check_structure(&ref_mut_ref_token), Some(VMStaticViolation::InvalidSignatureToken( ref_mut_ref_token.clone(), SignatureTokenKind::Reference, diff --git a/language/vm/src/checks.rs b/language/vm/src/checks.rs index f29e32fd7dff..d74ed53ea3f7 100644 --- a/language/vm/src/checks.rs +++ b/language/vm/src/checks.rs @@ -2,7 +2,5 @@ // SPDX-License-Identifier: Apache-2.0 pub mod bounds; -pub mod signature; pub use bounds::{BoundsCheck, BoundsChecker}; -pub use signature::SignatureCheck; diff --git a/language/vm/src/checks/signature.rs b/language/vm/src/checks/signature.rs deleted file mode 100644 index 03dde45e84ae..000000000000 --- a/language/vm/src/checks/signature.rs +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright (c) The Libra Core Contributors -// SPDX-License-Identifier: Apache-2.0 - -use crate::{ - access::ModuleAccess, - errors::VMStaticViolation, - file_format::{CompiledModule, SignatureToken}, - views::{ - FieldDefinitionView, FunctionSignatureView, LocalsSignatureView, SignatureTokenView, - TypeSignatureView, ViewInternals, - }, - SignatureTokenKind, -}; - -pub trait SignatureCheck { - fn check_signatures(&self) -> Vec; -} - -impl<'a, T: ModuleAccess> SignatureCheck for FunctionSignatureView<'a, T> { - fn check_signatures(&self) -> Vec { - self.return_tokens() - .filter_map(|token| token.check_structure()) - .chain( - self.arg_tokens() - .filter_map(|token| token.check_structure()), - ) - .collect() - } -} - -impl<'a, T: ModuleAccess> SignatureCheck for TypeSignatureView<'a, T> { - fn check_signatures(&self) -> Vec { - self.token().check_structure().into_iter().collect() - } -} - -impl<'a, T: ModuleAccess> SignatureCheck for LocalsSignatureView<'a, T> { - fn check_signatures(&self) -> Vec { - self.tokens() - .filter_map(|token| token.check_structure()) - .collect() - } -} - -impl<'a> FieldDefinitionView<'a, CompiledModule> { - /// Field definitions have additional constraints on signatures -- field signatures cannot be - /// references or mutable references. - pub fn check_signature_refs(&self) -> Option { - let type_signature = self.type_signature(); - let token = type_signature.token(); - let kind = token.kind(); - match kind { - SignatureTokenKind::Reference | SignatureTokenKind::MutableReference => Some( - VMStaticViolation::InvalidFieldDefReference(token.as_inner().clone(), kind), - ), - SignatureTokenKind::Value => None, - } - } -} - -impl<'a, T: ModuleAccess> SignatureTokenView<'a, T> { - /// Check that this token is structurally correct. - /// In particular, check that the token has a reference only at the top level. - #[inline] - pub fn check_structure(&self) -> Option { - self.as_inner().check_structure() - } -} - -impl SignatureToken { - // See SignatureTokenView::check_structure for more details. - pub(crate) fn check_structure(&self) -> Option { - use SignatureToken::*; - - let inner_token_opt = match self { - Reference(token) => Some(token), - MutableReference(token) => Some(token), - Bool | U64 | String | ByteArray | Address | Struct(_) => None, - }; - if let Some(inner_token) = inner_token_opt { - if inner_token.is_reference() { - return Some(VMStaticViolation::InvalidSignatureToken( - self.clone(), - self.kind(), - inner_token.kind(), - )); - } - } - None - } -} diff --git a/language/vm/src/unit_tests/mod.rs b/language/vm/src/unit_tests/mod.rs index 1824b979c657..f425ac3482cc 100644 --- a/language/vm/src/unit_tests/mod.rs +++ b/language/vm/src/unit_tests/mod.rs @@ -1,7 +1,6 @@ // Copyright (c) The Libra Core Contributors // SPDX-License-Identifier: Apache-2.0 -mod checks; mod deserializer_tests; mod fixture_tests; mod number_tests;