Skip to content
This repository was archived by the owner on Jan 10, 2025. It is now read-only.

Commit 32b1a6f

Browse files
authored
Refactor - Improve syscall errors (#536)
* Implements StableResult::map() and StableResult::map_err(). * Moves StableResult and ProgramResult from vm.rs into error.rs * Moves tests into the files where they belong. * Makes rust interface of syscalls return Box<dyn std::error::Error>.
1 parent ae16614 commit 32b1a6f

File tree

10 files changed

+180
-152
lines changed

10 files changed

+180
-152
lines changed

src/debugger.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@ use gdbstub::target::ext::section_offsets::Offsets;
2323

2424
use crate::{
2525
ebpf,
26-
error::EbpfError,
26+
error::{EbpfError, ProgramResult},
2727
interpreter::{DebugState, Interpreter},
2828
memory_region::AccessType,
29-
vm::{ContextObject, ProgramResult},
29+
vm::ContextObject,
3030
};
3131

3232
type DynResult<T> = Result<T, Box<dyn std::error::Error>>;

src/elf.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1189,10 +1189,11 @@ mod test {
11891189
consts::{ELFCLASS32, ELFDATA2MSB, ET_REL},
11901190
types::{Elf64Ehdr, Elf64Shdr},
11911191
},
1192+
error::ProgramResult,
11921193
fuzz::fuzz,
11931194
program::BuiltinFunction,
11941195
syscalls,
1195-
vm::{ProgramResult, TestContextObject},
1196+
vm::TestContextObject,
11961197
};
11971198
use rand::{distributions::Uniform, Rng};
11981199
use std::{fs::File, io::Read};

src/error.rs

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,3 +87,109 @@ pub enum EbpfError {
8787
#[error("Syscall error: {0}")]
8888
SyscallError(Box<dyn Error>),
8989
}
90+
91+
/// Same as `Result` but provides a stable memory layout
92+
#[derive(Debug)]
93+
#[repr(C, u64)]
94+
pub enum StableResult<T, E> {
95+
/// Success
96+
Ok(T),
97+
/// Failure
98+
Err(E),
99+
}
100+
101+
impl<T: std::fmt::Debug, E: std::fmt::Debug> StableResult<T, E> {
102+
/// `true` if `Ok`
103+
pub fn is_ok(&self) -> bool {
104+
match self {
105+
Self::Ok(_) => true,
106+
Self::Err(_) => false,
107+
}
108+
}
109+
110+
/// `true` if `Err`
111+
pub fn is_err(&self) -> bool {
112+
match self {
113+
Self::Ok(_) => false,
114+
Self::Err(_) => true,
115+
}
116+
}
117+
118+
/// Returns the inner value if `Ok`, panics otherwise
119+
pub fn unwrap(self) -> T {
120+
match self {
121+
Self::Ok(value) => value,
122+
Self::Err(error) => panic!("unwrap {:?}", error),
123+
}
124+
}
125+
126+
/// Returns the inner error if `Err`, panics otherwise
127+
pub fn unwrap_err(self) -> E {
128+
match self {
129+
Self::Ok(value) => panic!("unwrap_err {:?}", value),
130+
Self::Err(error) => error,
131+
}
132+
}
133+
134+
/// Maps ok values, leaving error values untouched
135+
pub fn map<U, O: FnOnce(T) -> U>(self, op: O) -> StableResult<U, E> {
136+
match self {
137+
Self::Ok(value) => StableResult::<U, E>::Ok(op(value)),
138+
Self::Err(error) => StableResult::<U, E>::Err(error),
139+
}
140+
}
141+
142+
/// Maps error values, leaving ok values untouched
143+
pub fn map_err<F, O: FnOnce(E) -> F>(self, op: O) -> StableResult<T, F> {
144+
match self {
145+
Self::Ok(value) => StableResult::<T, F>::Ok(value),
146+
Self::Err(error) => StableResult::<T, F>::Err(op(error)),
147+
}
148+
}
149+
150+
#[cfg_attr(
151+
any(
152+
not(feature = "jit"),
153+
target_os = "windows",
154+
not(target_arch = "x86_64")
155+
),
156+
allow(dead_code)
157+
)]
158+
pub(crate) fn discriminant(&self) -> u64 {
159+
unsafe { *(self as *const _ as *const u64) }
160+
}
161+
}
162+
163+
impl<T, E> From<StableResult<T, E>> for Result<T, E> {
164+
fn from(result: StableResult<T, E>) -> Self {
165+
match result {
166+
StableResult::Ok(value) => Ok(value),
167+
StableResult::Err(value) => Err(value),
168+
}
169+
}
170+
}
171+
172+
impl<T, E> From<Result<T, E>> for StableResult<T, E> {
173+
fn from(result: Result<T, E>) -> Self {
174+
match result {
175+
Ok(value) => Self::Ok(value),
176+
Err(value) => Self::Err(value),
177+
}
178+
}
179+
}
180+
181+
/// Return value of programs and syscalls
182+
pub type ProgramResult = StableResult<u64, EbpfError>;
183+
184+
#[cfg(test)]
185+
mod tests {
186+
use super::*;
187+
188+
#[test]
189+
fn test_program_result_is_stable() {
190+
let ok = ProgramResult::Ok(42);
191+
assert_eq!(ok.discriminant(), 0);
192+
let err = ProgramResult::Err(EbpfError::JitNotCompiled);
193+
assert_eq!(err.discriminant(), 1);
194+
}
195+
}

src/interpreter.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@
1515
use crate::{
1616
ebpf::{self, STACK_PTR_REG},
1717
elf::Executable,
18-
error::EbpfError,
19-
vm::{get_runtime_environment_key, Config, ContextObject, EbpfVm, ProgramResult},
18+
error::{EbpfError, ProgramResult},
19+
vm::{get_runtime_environment_key, Config, ContextObject, EbpfVm},
2020
};
2121

2222
/// Virtual memory operation helper.

src/jit.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,12 @@ use std::{fmt::Debug, mem, ptr};
1616
use crate::{
1717
ebpf::{self, FIRST_SCRATCH_REG, FRAME_PTR_REG, INSN_SIZE, SCRATCH_REGS, STACK_PTR_REG},
1818
elf::Executable,
19-
error::EbpfError,
19+
error::{EbpfError, ProgramResult},
2020
memory_management::{
2121
allocate_pages, free_pages, get_system_page_size, protect_pages, round_to_page_size,
2222
},
2323
memory_region::{AccessType, MemoryMapping},
24-
vm::{get_runtime_environment_key, Config, ContextObject, EbpfVm, ProgramResult},
24+
vm::{get_runtime_environment_key, Config, ContextObject, EbpfVm},
2525
x86::*,
2626
};
2727

src/memory_region.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
use crate::{
44
aligned_memory::Pod,
55
ebpf,
6-
error::EbpfError,
6+
error::{EbpfError, ProgramResult},
77
program::SBPFVersion,
8-
vm::{Config, ProgramResult},
8+
vm::Config,
99
};
1010
use std::{
1111
array,

src/program.rs

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ macro_rules! declare_builtin_function {
296296
$arg_d:ident : u64,
297297
$arg_e:ident : u64,
298298
$memory_mapping:ident : &mut $MemoryMapping:ty,
299-
) -> Result<u64, EbpfError> $rust:tt) => {
299+
) -> Result<u64, Box<dyn std::error::Error>> $rust:tt) => {
300300
$(#[$attr])*
301301
pub struct $name {}
302302
impl $name {
@@ -309,7 +309,7 @@ macro_rules! declare_builtin_function {
309309
$arg_d: u64,
310310
$arg_e: u64,
311311
$memory_mapping: &mut $MemoryMapping,
312-
) -> Result<u64, EbpfError> {
312+
) -> Result<u64, Box<dyn std::error::Error>> {
313313
$rust
314314
}
315315
/// VM interface
@@ -330,9 +330,9 @@ macro_rules! declare_builtin_function {
330330
if config.enable_instruction_meter {
331331
vm.context_object_pointer.consume(vm.previous_instruction_meter - vm.due_insn_count);
332332
}
333-
let converted_result: $crate::vm::ProgramResult = Self::rust(
333+
let converted_result: $crate::error::ProgramResult = Self::rust(
334334
vm.context_object_pointer, $arg_a, $arg_b, $arg_c, $arg_d, $arg_e, &mut vm.memory_mapping,
335-
).into();
335+
).map_err(|err| $crate::error::EbpfError::SyscallError(err)).into();
336336
vm.program_result = converted_result;
337337
if config.enable_instruction_meter {
338338
vm.previous_instruction_meter = vm.context_object_pointer.get_remaining();
@@ -341,3 +341,39 @@ macro_rules! declare_builtin_function {
341341
}
342342
};
343343
}
344+
345+
#[cfg(test)]
346+
mod tests {
347+
use super::*;
348+
use crate::{program::BuiltinFunction, syscalls, vm::TestContextObject};
349+
350+
#[test]
351+
fn test_builtin_program_eq() {
352+
let mut function_registry_a =
353+
FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
354+
function_registry_a
355+
.register_function_hashed(*b"log", syscalls::SyscallString::vm)
356+
.unwrap();
357+
function_registry_a
358+
.register_function_hashed(*b"log_64", syscalls::SyscallU64::vm)
359+
.unwrap();
360+
let mut function_registry_b =
361+
FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
362+
function_registry_b
363+
.register_function_hashed(*b"log_64", syscalls::SyscallU64::vm)
364+
.unwrap();
365+
function_registry_b
366+
.register_function_hashed(*b"log", syscalls::SyscallString::vm)
367+
.unwrap();
368+
let mut function_registry_c =
369+
FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
370+
function_registry_c
371+
.register_function_hashed(*b"log_64", syscalls::SyscallU64::vm)
372+
.unwrap();
373+
let builtin_program_a = BuiltinProgram::new_loader(Config::default(), function_registry_a);
374+
let builtin_program_b = BuiltinProgram::new_loader(Config::default(), function_registry_b);
375+
assert_eq!(builtin_program_a, builtin_program_b);
376+
let builtin_program_c = BuiltinProgram::new_loader(Config::default(), function_registry_c);
377+
assert_ne!(builtin_program_a, builtin_program_c);
378+
}
379+
}

src/syscalls.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ declare_builtin_function!(
4141
arg4: u64,
4242
arg5: u64,
4343
_memory_mapping: &mut MemoryMapping,
44-
) -> Result<u64, EbpfError> {
44+
) -> Result<u64, Box<dyn std::error::Error>> {
4545
println!("bpf_trace_printf: {arg3:#x}, {arg4:#x}, {arg5:#x}");
4646
let size_arg = |x| {
4747
if x == 0 {
@@ -69,7 +69,7 @@ declare_builtin_function!(
6969
arg4: u64,
7070
arg5: u64,
7171
_memory_mapping: &mut MemoryMapping,
72-
) -> Result<u64, EbpfError> {
72+
) -> Result<u64, Box<dyn std::error::Error>> {
7373
Ok(arg1.wrapping_shl(32)
7474
| arg2.wrapping_shl(24)
7575
| arg3.wrapping_shl(16)
@@ -91,7 +91,7 @@ declare_builtin_function!(
9191
_arg4: u64,
9292
_arg5: u64,
9393
memory_mapping: &mut MemoryMapping,
94-
) -> Result<u64, EbpfError> {
94+
) -> Result<u64, Box<dyn std::error::Error>> {
9595
let host_addr: Result<u64, EbpfError> =
9696
memory_mapping.map(AccessType::Store, vm_addr, len).into();
9797
let host_addr = host_addr?;
@@ -116,7 +116,7 @@ declare_builtin_function!(
116116
_arg4: u64,
117117
_arg5: u64,
118118
memory_mapping: &mut MemoryMapping,
119-
) -> Result<u64, EbpfError> {
119+
) -> Result<u64, Box<dyn std::error::Error>> {
120120
// C-like strcmp, maybe shorter than converting the bytes to string and comparing?
121121
if arg1 == 0 || arg2 == 0 {
122122
return Ok(u64::MAX);
@@ -154,7 +154,7 @@ declare_builtin_function!(
154154
_arg4: u64,
155155
_arg5: u64,
156156
memory_mapping: &mut MemoryMapping,
157-
) -> Result<u64, EbpfError> {
157+
) -> Result<u64, Box<dyn std::error::Error>> {
158158
let host_addr: Result<u64, EbpfError> =
159159
memory_mapping.map(AccessType::Load, vm_addr, len).into();
160160
let host_addr = host_addr?;
@@ -185,7 +185,7 @@ declare_builtin_function!(
185185
arg4: u64,
186186
arg5: u64,
187187
memory_mapping: &mut MemoryMapping,
188-
) -> Result<u64, EbpfError> {
188+
) -> Result<u64, Box<dyn std::error::Error>> {
189189
println!(
190190
"dump_64: {:#x}, {:#x}, {:#x}, {:#x}, {:#x}, {:?}",
191191
arg1, arg2, arg3, arg4, arg5, memory_mapping as *const _

0 commit comments

Comments
 (0)