More test fixes

solana-labs · Oct 14, 2024 · 624aac3 · 624aac3
1 parent 6ae150c
commit 624aac3
Show file tree

Hide file tree

Showing 13 changed files with 113 additions and 44 deletions.
diff --git a/benches/elf_loader.rs b/benches/elf_loader.rs
@@ -10,6 +10,7 @@ extern crate solana_rbpf;
 extern crate test;
 extern crate test_utils;
 
+use solana_rbpf::program::SyscallRegistry;
 use solana_rbpf::{
     elf::Executable,
     program::{BuiltinFunction, BuiltinProgram, FunctionRegistry},
@@ -27,6 +28,7 @@ fn loader() -> Arc<BuiltinProgram<TestContextObject>> {
     Arc::new(BuiltinProgram::new_loader(
         Config::default(),
         function_registry,
+        SyscallRegistry::default(),
     ))
 }
 

diff --git a/fuzz/fuzz_targets/dumb.rs b/fuzz/fuzz_targets/dumb.rs
@@ -8,7 +8,7 @@ use solana_rbpf::{
     ebpf,
     elf::Executable,
     memory_region::MemoryRegion,
-    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion},
+    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SyscallRegistry, SBPFVersion},
     verifier::{RequisiteVerifier, Verifier},
     vm::TestContextObject,
 };
@@ -29,7 +29,7 @@ fuzz_target!(|data: DumbFuzzData| {
     let prog = data.prog;
     let config = data.template.into();
     let function_registry = FunctionRegistry::default();
-    let syscall_registry = FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
+    let syscall_registry = SyscallRegistry::<BuiltinFunction<TestContextObject>>::default();
 
     if RequisiteVerifier::verify(&prog, &config, &SBPFVersion::V2, &function_registry, &syscall_registry).is_err() {
         // verify please
@@ -41,6 +41,7 @@ fuzz_target!(|data: DumbFuzzData| {
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
             FunctionRegistry::default(),
+            syscall_registry,
         )),
         SBPFVersion::V2,
         function_registry,

diff --git a/fuzz/fuzz_targets/smart.rs b/fuzz/fuzz_targets/smart.rs
@@ -10,7 +10,7 @@ use solana_rbpf::{
     elf::Executable,
     insn_builder::{Arch, IntoBytes},
     memory_region::MemoryRegion,
-    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion},
+    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SyscallRegistry, SBPFVersion},
     verifier::{RequisiteVerifier, Verifier},
     vm::TestContextObject,
 };
@@ -33,7 +33,7 @@ fuzz_target!(|data: FuzzData| {
     let prog = make_program(&data.prog, data.arch);
     let config = data.template.into();
     let function_registry = FunctionRegistry::default();
-    let syscall_registry = FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
+    let syscall_registry = SyscallRegistry::<BuiltinFunction<TestContextObject>>::default();
 
     if RequisiteVerifier::verify(
         prog.into_bytes(),
@@ -53,6 +53,7 @@ fuzz_target!(|data: FuzzData| {
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
             FunctionRegistry::default(),
+            syscall_registry,
         )),
         SBPFVersion::V2,
         function_registry,

diff --git a/fuzz/fuzz_targets/smart_jit_diff.rs b/fuzz/fuzz_targets/smart_jit_diff.rs
@@ -8,7 +8,7 @@ use solana_rbpf::{
     elf::Executable,
     insn_builder::{Arch, Instruction, IntoBytes},
     memory_region::MemoryRegion,
-    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion},
+    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SyscallRegistry, SBPFVersion},
     verifier::{RequisiteVerifier, Verifier},
     vm::TestContextObject,
 };
@@ -40,7 +40,7 @@ fuzz_target!(|data: FuzzData| {
         .push();
     let config = data.template.into();
     let function_registry = FunctionRegistry::default();
-    let syscall_registry = FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
+    let syscall_registry = SyscallRegistry::<BuiltinFunction<TestContextObject>>::default();
 
     if RequisiteVerifier::verify(
         prog.into_bytes(),
@@ -61,6 +61,7 @@ fuzz_target!(|data: FuzzData| {
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
             FunctionRegistry::default(),
+            syscall_registry,
         )),
         SBPFVersion::V2,
         function_registry,

diff --git a/fuzz/fuzz_targets/smarter_jit_diff.rs b/fuzz/fuzz_targets/smarter_jit_diff.rs
@@ -8,7 +8,7 @@ use solana_rbpf::{
     elf::Executable,
     insn_builder::IntoBytes,
     memory_region::MemoryRegion,
-    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion},
+    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SyscallRegistry, SBPFVersion},
     verifier::{RequisiteVerifier, Verifier},
     vm::TestContextObject,
 };
@@ -30,7 +30,7 @@ fuzz_target!(|data: FuzzData| {
     let prog = make_program(&data.prog);
     let config = data.template.into();
     let function_registry = FunctionRegistry::default();
-    let syscall_registry = FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
+    let syscall_registry = SyscallRegistry::<BuiltinFunction<TestContextObject>>::default();
 
     if RequisiteVerifier::verify(
         prog.into_bytes(),
@@ -51,6 +51,7 @@ fuzz_target!(|data: FuzzData| {
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
             FunctionRegistry::default(),
+            syscall_registry,
         )),
         SBPFVersion::V2,
         function_registry,

diff --git a/fuzz/fuzz_targets/verify_semantic_aware.rs b/fuzz/fuzz_targets/verify_semantic_aware.rs
@@ -5,7 +5,7 @@ use libfuzzer_sys::fuzz_target;
 use semantic_aware::*;
 use solana_rbpf::{
     insn_builder::IntoBytes,
-    program::{BuiltinFunction, FunctionRegistry, SBPFVersion},
+    program::{BuiltinFunction, FunctionRegistry, SyscallRegistry, SBPFVersion},
     verifier::{RequisiteVerifier, Verifier},
     vm::TestContextObject,
 };
@@ -25,7 +25,7 @@ fuzz_target!(|data: FuzzData| {
     let prog = make_program(&data.prog);
     let config = data.template.into();
     let function_registry = FunctionRegistry::default();
-    let syscall_registry = FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
+    let syscall_registry = SyscallRegistry::<BuiltinFunction<TestContextObject>>::default();
 
     RequisiteVerifier::verify(
         prog.into_bytes(),

diff --git a/src/elf.rs b/src/elf.rs
@@ -1147,6 +1147,7 @@ pub(crate) fn get_ro_region(ro_section: &Section, elf: &[u8]) -> MemoryRegion {
 #[cfg(test)]
 mod test {
     use super::*;
+    use crate::program::SyscallRegistry;
     use crate::{
         elf_parser::{
             // FIXME consts::{ELFCLASS32, ELFDATA2MSB, ET_REL},
@@ -1163,6 +1164,7 @@ mod test {
     use rand::{distributions::Uniform, Rng};
     use std::{fs::File, io::Read};
     use test_utils::assert_error;
+
     type ElfExecutable = Executable<TestContextObject>;
 
     fn loader() -> Arc<BuiltinProgram<TestContextObject>> {
@@ -1177,6 +1179,7 @@ mod test {
         Arc::new(BuiltinProgram::new_loader(
             Config::default(),
             function_registry,
+            SyscallRegistry::default(),
         ))
     }
 
@@ -1932,6 +1935,7 @@ mod test {
                 ..Config::default()
             },
             FunctionRegistry::default(),
+            SyscallRegistry::default(),
         );
         let elf_bytes = std::fs::read("tests/elfs/syscall_reloc_64_32_sbpfv1.so")
             .expect("failed to read elf file");

diff --git a/src/program.rs b/src/program.rs
@@ -228,13 +228,11 @@ impl<T: Copy + PartialEq> Default for SyscallRegistry<T> {
     }
 }
 
-impl <T: Copy + PartialEq> SyscallRegistry<T> {
+impl<T: Copy + PartialEq> SyscallRegistry<T> {
     /// Create a new registry based on a previously created vector of registrations.
     /// We expect the vector to be dense, containing one function per position.
     pub fn new(registry: Vec<(Vec<u8>, T)>) -> Self {
-        SyscallRegistry {
-            look_up: registry,
-        }
+        SyscallRegistry { look_up: registry }
     }
 
     /// Retrieve a syscall from the registry
@@ -243,9 +241,12 @@ impl <T: Copy + PartialEq> SyscallRegistry<T> {
             return Err(ElfError::InvalidSyscallCode);
         }
 
-        self.look_up.get(code-1).as_ref()
-            .map_or(Err(ElfError::InvalidSyscallCode),
-                    |(name, value)| Ok((name.as_slice(), *value)))
+        self.look_up
+            .get(code.saturating_sub(1))
+            .as_ref()
+            .map_or(Err(ElfError::InvalidSyscallCode), |(name, value)| {
+                Ok((name.as_slice(), *value))
+            })
     }
 }
 
@@ -271,7 +272,11 @@ impl<C: ContextObject> PartialEq for BuiltinProgram<C> {
 
 impl<C: ContextObject> BuiltinProgram<C> {
     /// Constructs a loader built-in program
-    pub fn new_loader(config: Config, functions: FunctionRegistry<BuiltinFunction<C>>, syscalls: SyscallRegistry<BuiltinFunction<C>>) -> Self {
+    pub fn new_loader(
+        config: Config,
+        functions: FunctionRegistry<BuiltinFunction<C>>,
+        syscalls: SyscallRegistry<BuiltinFunction<C>>,
+    ) -> Self {
         Self {
             config: Some(Box::new(config)),
             functions,
@@ -280,7 +285,10 @@ impl<C: ContextObject> BuiltinProgram<C> {
     }
 
     /// Constructs a built-in program
-    pub fn new_builtin(functions: FunctionRegistry<BuiltinFunction<C>>, syscalls: SyscallRegistry<BuiltinFunction<C>>) -> Self {
+    pub fn new_builtin(
+        functions: FunctionRegistry<BuiltinFunction<C>>,
+        syscalls: SyscallRegistry<BuiltinFunction<C>>,
+    ) -> Self {
         Self {
             config: None,
             functions,
@@ -421,19 +429,30 @@ mod tests {
         function_registry_c
             .register_function_hashed(*b"log_64", syscalls::SyscallU64::vm)
             .unwrap();
-        let builtin_program_a = BuiltinProgram::new_loader(Config::default(), function_registry_a);
-        let builtin_program_b = BuiltinProgram::new_loader(Config::default(), function_registry_b);
+        let builtin_program_a = BuiltinProgram::new_loader(
+            Config::default(),
+            function_registry_a,
+            SyscallRegistry::default(),
+        );
+        let builtin_program_b = BuiltinProgram::new_loader(
+            Config::default(),
+            function_registry_b,
+            SyscallRegistry::default(),
+        );
         assert_eq!(builtin_program_a, builtin_program_b);
-        let builtin_program_c = BuiltinProgram::new_loader(Config::default(), function_registry_c);
+        let builtin_program_c = BuiltinProgram::new_loader(
+            Config::default(),
+            function_registry_c,
+            SyscallRegistry::default(),
+        );
         assert_ne!(builtin_program_a, builtin_program_c);
     }
 
     #[test]
     fn test_syscall_registry() {
-        let registry_vec = vec![
-            (b"log".to_vec(), syscalls::SyscallString::vm)
-        ];
-        let registry = SyscallRegistry::new(registry_vec);
+        let registry_vec: Vec<(Vec<u8>, BuiltinFunction<TestContextObject>)> =
+            vec![(b"log".to_vec(), syscalls::SyscallString::vm)];
+        let registry = SyscallRegistry::<BuiltinFunction<TestContextObject>>::new(registry_vec);
 
         let lookup_result = registry.lookup_syscall(0);
         assert_eq!(lookup_result.err().unwrap(), ElfError::InvalidSyscallCode);

diff --git a/src/verifier.rs b/src/verifier.rs
@@ -12,13 +12,13 @@
 
 //! Verifies that the bytecode is valid for the given config.
 
+use crate::program::SyscallRegistry;
 use crate::{
     ebpf,
     program::{BuiltinFunction, FunctionRegistry, SBPFVersion},
     vm::{Config, ContextObject},
 };
 use thiserror::Error;
-use crate::program::SyscallRegistry;
 
 /// Error definitions
 #[derive(Debug, Error, Eq, PartialEq)]
@@ -214,12 +214,11 @@ fn check_callx_register(
 fn check_syscall<C: ContextObject>(
     code: usize,
     pc: usize,
-    registry: &SyscallRegistry<BuiltinFunction<C>>
+    registry: &SyscallRegistry<BuiltinFunction<C>>,
 ) -> Result<(), VerifierError> {
-    registry.lookup_syscall(code).map_or(
-        Err(VerifierError::InvalidFunction(pc)),
-        |_| Ok(())
-    )
+    registry
+        .lookup_syscall(code)
+        .map_or(Err(VerifierError::InvalidFunction(pc)), |_| Ok(()))
 }
 
 /// Mandatory verifier for solana programs to run on-chain

diff --git a/tests/assembler.rs b/tests/assembler.rs
@@ -8,7 +8,7 @@
 extern crate solana_rbpf;
 extern crate test_utils;
 
-use solana_rbpf::program::{FunctionRegistry, SBPFVersion};
+use solana_rbpf::program::{FunctionRegistry, SBPFVersion, SyscallRegistry};
 use solana_rbpf::vm::Config;
 use solana_rbpf::{assembler::assemble, ebpf, program::BuiltinProgram, vm::TestContextObject};
 use std::sync::Arc;
@@ -19,7 +19,11 @@ fn asm(src: &str) -> Result<Vec<ebpf::Insn>, String> {
 }
 
 fn asm_with_config(src: &str, config: Config) -> Result<Vec<ebpf::Insn>, String> {
-    let loader = BuiltinProgram::new_loader(config, FunctionRegistry::default());
+    let loader = BuiltinProgram::new_loader(
+        config,
+        FunctionRegistry::default(),
+        SyscallRegistry::default(),
+    );
     let executable = assemble::<TestContextObject>(src, Arc::new(loader))?;
     let (_program_vm_addr, program) = executable.get_text_bytes();
     Ok((0..program.len() / ebpf::INSN_SIZE)
@@ -527,6 +531,7 @@ fn test_tcp_sack() {
         Arc::new(BuiltinProgram::new_loader(
             config,
             FunctionRegistry::default(),
+            SyscallRegistry::default(),
         )),
     )
     .unwrap();

diff --git a/tests/disassembler.rs b/tests/disassembler.rs
@@ -7,7 +7,7 @@
 // copied, modified, or distributed except according to those terms.
 
 extern crate solana_rbpf;
-use solana_rbpf::program::SBPFVersion;
+use solana_rbpf::program::{SBPFVersion, SyscallRegistry};
 use solana_rbpf::{
     assembler::assemble,
     program::{BuiltinProgram, FunctionRegistry},
@@ -27,7 +27,11 @@ macro_rules! disasm {
     }};
     ($src:expr, $config:expr) => {{
         let src = $src;
-        let loader = BuiltinProgram::new_loader($config, FunctionRegistry::default());
+        let loader = BuiltinProgram::new_loader(
+            $config,
+            FunctionRegistry::default(),
+            SyscallRegistry::default(),
+        );
         let executable = assemble::<TestContextObject>(src, Arc::new(loader)).unwrap();
         let analysis = Analysis::from_executable(&executable).unwrap();
         let mut reasm = Vec::new();