From e621a09181d0a5ddb6289d8b13d4b89a71de63f1 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 11 Jul 2023 17:55:47 -0400 Subject: [PATCH] Clippy over tests and integration-ebpf Replace all `assert!(matches!(..))` with `assert_matches!(..)`. Remove the now-unused build-integration-test xtask command whose logic doesn't match that of the build-and-run command. --- .github/workflows/lint.yml | 9 +-- aya-obj/src/obj.rs | 49 ++++++-------- aya-obj/src/relocation.rs | 10 +-- aya/src/maps/bloom_filter.rs | 27 ++++---- aya/src/maps/hash_map/hash_map.rs | 67 ++++++++++--------- aya/src/maps/lpm_trie.rs | 32 ++++----- aya/src/maps/mod.rs | 10 ++- aya/src/maps/perf/perf_buffer.rs | 19 +++--- aya/src/programs/links.rs | 10 ++- bpf/aya-bpf/src/helpers.rs | 4 +- test/integration-ebpf/src/bpf_probe_read.rs | 57 +++++++++------- test/integration-ebpf/src/log.rs | 3 +- test/integration-ebpf/src/map_test.rs | 3 +- test/integration-ebpf/src/name_test.rs | 3 +- test/integration-ebpf/src/pass.rs | 3 +- test/integration-ebpf/src/relocations.rs | 3 +- test/integration-ebpf/src/test.rs | 3 +- test/integration-test/Cargo.toml | 9 ++- test/integration-test/tests/bpf_probe_read.rs | 20 +----- test/integration-test/tests/rbpf.rs | 9 +-- test/integration-test/tests/relocations.rs | 17 +---- test/run.sh | 6 +- xtask/src/build_test.rs | 32 --------- xtask/src/main.rs | 5 -- 24 files changed, 173 insertions(+), 237 deletions(-) delete mode 100644 xtask/src/build_test.rs diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5c68590f..713f36fa 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -26,13 +26,10 @@ jobs: components: rustfmt, clippy, miri, rust-src - name: Check formatting - run: | - cargo fmt --all -- --check + run: cargo fmt --all -- --check - name: Run clippy - run: | - cargo clippy --workspace --exclude integration-test -- --deny warnings + run: cargo clippy --all-targets --workspace --exclude integration-test -- --deny warnings - name: Run miri - run: | - cargo miri test --all-targets \ No newline at end of file + run: cargo miri test --all-targets diff --git a/aya-obj/src/obj.rs b/aya-obj/src/obj.rs index e5e2f3ab..d3ccb265 100644 --- a/aya-obj/src/obj.rs +++ b/aya-obj/src/obj.rs @@ -102,7 +102,7 @@ impl Features { } /// The loaded object file representation -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Object { /// The endianness pub endianness: Endianness, @@ -1537,66 +1537,57 @@ mod tests { #[test] fn test_parse_generic_error() { - assert!(matches!( - Object::parse(&b"foo"[..]), - Err(ParseError::ElfError(_)) - )) + assert_matches!(Object::parse(&b"foo"[..]), Err(ParseError::ElfError(_))) } #[test] fn test_parse_license() { - assert!(matches!( - parse_license(b""), - Err(ParseError::InvalidLicense { .. }) - )); + assert_matches!(parse_license(b""), Err(ParseError::InvalidLicense { .. })); - assert!(matches!( - parse_license(b"\0"), - Err(ParseError::InvalidLicense { .. }) - )); + assert_matches!(parse_license(b"\0"), Err(ParseError::InvalidLicense { .. })); - assert!(matches!( + assert_matches!( parse_license(b"GPL"), Err(ParseError::MissingLicenseNullTerminator { .. }) - )); + ); assert_eq!(parse_license(b"GPL\0").unwrap().to_str().unwrap(), "GPL"); } #[test] fn test_parse_version() { - assert!(matches!( + assert_matches!( parse_version(b"", Endianness::Little), Err(ParseError::InvalidKernelVersion { .. }) - )); + ); - assert!(matches!( + assert_matches!( parse_version(b"123", Endianness::Little), Err(ParseError::InvalidKernelVersion { .. }) - )); + ); - assert!(matches!( + assert_matches!( parse_version(&0xFFFF_FFFEu32.to_le_bytes(), Endianness::Little), Ok(None) - )); + ); - assert!(matches!( + assert_matches!( parse_version(&0xFFFF_FFFEu32.to_be_bytes(), Endianness::Big), Ok(None) - )); + ); - assert!(matches!( + assert_matches!( parse_version(&1234u32.to_le_bytes(), Endianness::Little), Ok(Some(1234)) - )); + ); } #[test] fn test_parse_map_def_error() { - assert!(matches!( + assert_matches!( parse_map_def("foo", &[]), Err(ParseError::InvalidMapDefinition { .. }) - )); + ); } #[test] @@ -1652,7 +1643,7 @@ mod tests { #[test] fn test_parse_map_data() { let map_data = b"map data"; - assert!(matches!( + assert_matches!( parse_data_map_section( &fake_section( BpfSectionKind::Data, @@ -1675,7 +1666,7 @@ mod tests { }, data, })) if data == map_data && value_size == map_data.len() as u32 - )) + ) } fn fake_obj() -> Object { diff --git a/aya-obj/src/relocation.rs b/aya-obj/src/relocation.rs index f310557e..fc02a79f 100644 --- a/aya-obj/src/relocation.rs +++ b/aya-obj/src/relocation.rs @@ -261,7 +261,7 @@ fn relocate_maps<'a, I: Iterator>( debug_assert!(matches!( map.section_kind(), BpfSectionKind::Bss | BpfSectionKind::Data | BpfSectionKind::Rodata - ),); + )); m }; debug_assert_eq!(map.section_index(), section_index); @@ -580,7 +580,7 @@ mod test { let symbol_table = HashMap::from([(1, fake_sym(1, 0, 0, "test_map", 0))]); - let relocations = vec![Relocation { + let relocations = [Relocation { offset: 0x0, symbol_index: 1, size: 64, @@ -625,7 +625,7 @@ mod test { (2, fake_sym(2, 0, 0, "test_map_2", 0)), ]); - let relocations = vec![ + let relocations = [ Relocation { offset: 0x0, symbol_index: 1, @@ -675,7 +675,7 @@ mod test { let symbol_table = HashMap::from([(1, fake_sym(1, 0, 0, "test_map", 0))]); - let relocations = vec![Relocation { + let relocations = [Relocation { offset: 0x0, symbol_index: 1, size: 64, @@ -720,7 +720,7 @@ mod test { (2, fake_sym(2, 0, 0, "test_map_2", 0)), ]); - let relocations = vec![ + let relocations = [ Relocation { offset: 0x0, symbol_index: 1, diff --git a/aya/src/maps/bloom_filter.rs b/aya/src/maps/bloom_filter.rs index bdf81eae..315fe7d4 100644 --- a/aya/src/maps/bloom_filter.rs +++ b/aya/src/maps/bloom_filter.rs @@ -30,6 +30,7 @@ use crate::{ /// ``` #[doc(alias = "BPF_MAP_TYPE_BLOOM_FILTER")] +#[derive(Debug)] pub struct BloomFilter { inner: T, _v: PhantomData, @@ -88,6 +89,7 @@ mod tests { sys::{override_syscall, SysResult, Syscall}, }; use libc::{EFAULT, ENOENT}; + use matches::assert_matches; use std::io; fn new_obj_map() -> obj::Map { @@ -118,13 +120,13 @@ mod tests { pinned: false, btf_fd: None, }; - assert!(matches!( + assert_matches!( BloomFilter::<_, u16>::new(&map), Err(MapError::InvalidValueSize { size: 2, expected: 4 }) - )); + ); } #[test] @@ -150,10 +152,10 @@ mod tests { let map = Map::PerfEventArray(map_data); - assert!(matches!( + assert_matches!( BloomFilter::<_, u32>::try_from(&map), Err(MapError::InvalidMapType { .. }) - )); + ); } #[test] @@ -165,10 +167,10 @@ mod tests { btf_fd: None, }; - assert!(matches!( + assert_matches!( BloomFilter::<_, u32>::new(&mut map), Err(MapError::NotCreated { .. }) - )); + ); } #[test] @@ -208,10 +210,10 @@ mod tests { }; let bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); - assert!(matches!( + assert_matches!( bloom_filter.insert(1, 0), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_push_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -246,10 +248,10 @@ mod tests { }; let bloom_filter = BloomFilter::<_, u32>::new(&map).unwrap(); - assert!(matches!( + assert_matches!( bloom_filter.contains(&1, 0), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_lookup_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -269,9 +271,6 @@ mod tests { }; let bloom_filter = BloomFilter::<_, u32>::new(&map).unwrap(); - assert!(matches!( - bloom_filter.contains(&1, 0), - Err(MapError::ElementNotFound) - )); + assert_matches!(bloom_filter.contains(&1, 0), Err(MapError::ElementNotFound)); } } diff --git a/aya/src/maps/hash_map/hash_map.rs b/aya/src/maps/hash_map/hash_map.rs index 493a944b..b37e5c8f 100644 --- a/aya/src/maps/hash_map/hash_map.rs +++ b/aya/src/maps/hash_map/hash_map.rs @@ -108,6 +108,7 @@ mod tests { use std::io; use libc::{EFAULT, ENOENT}; + use matches::assert_matches; use crate::{ bpf_map_def, @@ -150,13 +151,13 @@ mod tests { pinned: false, btf_fd: None, }; - assert!(matches!( + assert_matches!( HashMap::<_, u8, u32>::new(&map), Err(MapError::InvalidKeySize { size: 1, expected: 4 }) - )); + ); } #[test] @@ -167,13 +168,13 @@ mod tests { pinned: false, btf_fd: None, }; - assert!(matches!( + assert_matches!( HashMap::<_, u32, u16>::new(&map), Err(MapError::InvalidValueSize { size: 2, expected: 4 }) - )); + ); } #[test] @@ -186,10 +187,10 @@ mod tests { }; let map = Map::Array(map_data); - assert!(matches!( + assert_matches!( HashMap::<_, u8, u32>::try_from(&map), Err(MapError::InvalidMapType { .. }) - )); + ); } #[test] @@ -202,13 +203,13 @@ mod tests { }; let map = Map::HashMap(map_data); - assert!(matches!( + assert_matches!( HashMap::<_, u32, u16>::try_from(&map), Err(MapError::InvalidValueSize { size: 2, expected: 4 }) - )); + ); } #[test] @@ -220,10 +221,10 @@ mod tests { btf_fd: None, }; - assert!(matches!( + assert_matches!( HashMap::<_, u32, u32>::new(&mut map), Err(MapError::NotCreated { .. }) - )); + ); } #[test] @@ -289,10 +290,10 @@ mod tests { }; let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); - assert!(matches!( + assert_matches!( hm.insert(1, 42, 0), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_update_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -349,10 +350,10 @@ mod tests { }; let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); - assert!(matches!( + assert_matches!( hm.remove(&1), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_delete_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -387,10 +388,10 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - assert!(matches!( + assert_matches!( hm.get(&1, 0), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_lookup_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -410,7 +411,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - assert!(matches!(hm.get(&1, 0), Err(MapError::KeyNotFound))); + assert_matches!(hm.get(&1, 0), Err(MapError::KeyNotFound)); } fn bpf_key(attr: &bpf_attr) -> Option { @@ -447,7 +448,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let keys = hm.keys().collect::, _>>(); - assert!(matches!(keys, Ok(ks) if ks.is_empty())) + assert_matches!(keys, Ok(ks) if ks.is_empty()) } fn get_next_key(attr: &bpf_attr) -> SysResult { @@ -530,13 +531,13 @@ mod tests { let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let mut keys = hm.keys(); - assert!(matches!(keys.next(), Some(Ok(10)))); - assert!(matches!(keys.next(), Some(Ok(20)))); - assert!(matches!( + assert_matches!(keys.next(), Some(Ok(10))); + assert_matches!(keys.next(), Some(Ok(20))); + assert_matches!( keys.next(), Some(Err(MapError::SyscallError { call, .. })) if call == "bpf_map_get_next_key" - )); - assert!(matches!(keys.next(), None)); + ); + assert_matches!(keys.next(), None); } #[test] @@ -642,13 +643,13 @@ mod tests { let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let mut iter = hm.iter(); - assert!(matches!(iter.next(), Some(Ok((10, 100))))); - assert!(matches!(iter.next(), Some(Ok((20, 200))))); - assert!(matches!( + assert_matches!(iter.next(), Some(Ok((10, 100)))); + assert_matches!(iter.next(), Some(Ok((20, 200)))); + assert_matches!( iter.next(), Some(Err(MapError::SyscallError { call, .. })) if call == "bpf_map_get_next_key" - )); - assert!(matches!(iter.next(), None)); + ); + assert_matches!(iter.next(), None); } #[test] @@ -687,12 +688,12 @@ mod tests { let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let mut iter = hm.iter(); - assert!(matches!(iter.next(), Some(Ok((10, 100))))); - assert!(matches!( + assert_matches!(iter.next(), Some(Ok((10, 100)))); + assert_matches!( iter.next(), Some(Err(MapError::SyscallError { call, .. })) if call == "bpf_map_lookup_elem" - )); - assert!(matches!(iter.next(), Some(Ok((30, 300))))); - assert!(matches!(iter.next(), None)); + ); + assert_matches!(iter.next(), Some(Ok((30, 300)))); + assert_matches!(iter.next(), None); } } diff --git a/aya/src/maps/lpm_trie.rs b/aya/src/maps/lpm_trie.rs index 0d8b9d09..f2fb10e3 100644 --- a/aya/src/maps/lpm_trie.rs +++ b/aya/src/maps/lpm_trie.rs @@ -45,6 +45,7 @@ use crate::{ /// ``` #[doc(alias = "BPF_MAP_TYPE_LPM_TRIE")] +#[derive(Debug)] pub struct LpmTrie { inner: T, _k: PhantomData, @@ -207,6 +208,7 @@ mod tests { sys::{override_syscall, SysResult, Syscall}, }; use libc::{EFAULT, ENOENT}; + use matches::assert_matches; use std::{io, mem, net::Ipv4Addr}; fn new_obj_map() -> obj::Map { @@ -237,13 +239,13 @@ mod tests { pinned: false, btf_fd: None, }; - assert!(matches!( + assert_matches!( LpmTrie::<_, u16, u32>::new(&map), Err(MapError::InvalidKeySize { size: 6, expected: 8 // four bytes for prefixlen and four bytes for data. }) - )); + ); } #[test] @@ -254,13 +256,13 @@ mod tests { pinned: false, btf_fd: None, }; - assert!(matches!( + assert_matches!( LpmTrie::<_, u32, u16>::new(&map), Err(MapError::InvalidValueSize { size: 2, expected: 4 }) - )); + ); } #[test] @@ -286,10 +288,10 @@ mod tests { let map = Map::PerfEventArray(map_data); - assert!(matches!( + assert_matches!( LpmTrie::<_, u32, u32>::try_from(&map), Err(MapError::InvalidMapType { .. }) - )); + ); } #[test] @@ -301,10 +303,10 @@ mod tests { btf_fd: None, }; - assert!(matches!( + assert_matches!( LpmTrie::<_, u32, u32>::new(&mut map), Err(MapError::NotCreated { .. }) - )); + ); } #[test] @@ -345,10 +347,10 @@ mod tests { let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); - assert!(matches!( + assert_matches!( trie.insert(&key, 1, 0), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_update_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -387,10 +389,10 @@ mod tests { let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); - assert!(matches!( + assert_matches!( trie.remove(&key), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_delete_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -428,10 +430,10 @@ mod tests { let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); - assert!(matches!( + assert_matches!( trie.get(&key, 0), Err(MapError::SyscallError { call, io_error }) if call == "bpf_map_lookup_elem" && io_error.raw_os_error() == Some(EFAULT) - )); + ); } #[test] @@ -453,6 +455,6 @@ mod tests { let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); - assert!(matches!(trie.get(&key, 0), Err(MapError::KeyNotFound))); + assert_matches!(trie.get(&key, 0), Err(MapError::KeyNotFound)); } } diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index 2332e1f5..24ac992c 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -833,6 +833,7 @@ impl Deref for PerCpuValues { #[cfg(test)] mod tests { use libc::EFAULT; + use matches::assert_matches; use crate::{ bpf_map_def, @@ -880,12 +881,9 @@ mod tests { }); let mut map = new_map(); - assert!(matches!(map.create("foo"), Ok(42))); + assert_matches!(map.create("foo"), Ok(42)); assert_eq!(map.fd, Some(42)); - assert!(matches!( - map.create("foo"), - Err(MapError::AlreadyCreated { .. }) - )); + assert_matches!(map.create("foo"), Err(MapError::AlreadyCreated { .. })); } #[test] @@ -894,7 +892,7 @@ mod tests { let mut map = new_map(); let ret = map.create("foo"); - assert!(matches!(ret, Err(MapError::CreateError { .. }))); + assert_matches!(ret, Err(MapError::CreateError { .. })); if let Err(MapError::CreateError { name, code, diff --git a/aya/src/maps/perf/perf_buffer.rs b/aya/src/maps/perf/perf_buffer.rs index 8db3ee4d..c5b47e3a 100644 --- a/aya/src/maps/perf/perf_buffer.rs +++ b/aya/src/maps/perf/perf_buffer.rs @@ -83,6 +83,7 @@ pub struct Events { pub lost: usize, } +#[derive(Debug)] pub(crate) struct PerfBuffer { buf: AtomicPtr, size: usize, @@ -318,6 +319,7 @@ mod tests { generated::perf_event_mmap_page, sys::{override_syscall, Syscall, TEST_MMAP_RET}, }; + use matches::assert_matches; use std::{fmt::Debug, mem}; const PAGE_SIZE: usize = 4096; @@ -336,18 +338,18 @@ mod tests { #[test] fn test_invalid_page_count() { - assert!(matches!( + assert_matches!( PerfBuffer::open(1, PAGE_SIZE, 0), Err(PerfBufferError::InvalidPageCount { .. }) - )); - assert!(matches!( + ); + assert_matches!( PerfBuffer::open(1, PAGE_SIZE, 3), Err(PerfBufferError::InvalidPageCount { .. }) - )); - assert!(matches!( + ); + assert_matches!( PerfBuffer::open(1, PAGE_SIZE, 5), Err(PerfBufferError::InvalidPageCount { .. }) - )); + ); } #[test] @@ -359,10 +361,7 @@ mod tests { fake_mmap(&mut mmapped_buf); let mut buf = PerfBuffer::open(1, PAGE_SIZE, 1).unwrap(); - assert!(matches!( - buf.read_events(&mut []), - Err(PerfBufferError::NoBuffers) - )) + assert_matches!(buf.read_events(&mut []), Err(PerfBufferError::NoBuffers)) } #[test] diff --git a/aya/src/programs/links.rs b/aya/src/programs/links.rs index 7e66af46..5f5c188f 100644 --- a/aya/src/programs/links.rs +++ b/aya/src/programs/links.rs @@ -329,6 +329,7 @@ pub enum LinkError { #[cfg(test)] mod tests { + use matches::assert_matches; use std::{cell::RefCell, env, fs::File, mem, os::unix::io::AsRawFd, rc::Rc}; use crate::{programs::ProgramError, sys::override_syscall}; @@ -394,10 +395,10 @@ mod tests { let mut links = LinkMap::new(); links.insert(TestLink::new(1, 2)).unwrap(); - assert!(matches!( + assert_matches!( links.insert(TestLink::new(1, 2)), Err(ProgramError::AlreadyAttached) - )); + ); } #[test] @@ -409,10 +410,7 @@ mod tests { let l1_id2 = l1.id(); links.insert(TestLink::new(1, 2)).unwrap(); links.remove(l1_id1).unwrap(); - assert!(matches!( - links.remove(l1_id2), - Err(ProgramError::NotAttached) - )); + assert_matches!(links.remove(l1_id2), Err(ProgramError::NotAttached)); } #[test] diff --git a/bpf/aya-bpf/src/helpers.rs b/bpf/aya-bpf/src/helpers.rs index 69ab3ccc..9543262b 100644 --- a/bpf/aya-bpf/src/helpers.rs +++ b/bpf/aya-bpf/src/helpers.rs @@ -438,7 +438,9 @@ fn read_str_bytes(len: i64, dest: &mut [u8]) -> Result<&[u8], c_long> { // len includes the NULL terminator but not for b"\0" for which the kernel // returns len=0. So we do a saturating sub and for b"\0" we return the // empty slice, for all other cases we omit the terminator. - Ok(&dest[..(len as usize).saturating_sub(1)]) + let len = usize::try_from(len).map_err(|core::num::TryFromIntError { .. }| -1)?; + let len = len.saturating_sub(1); + dest.get(..len).ok_or(-1) } /// Read a null-terminated string from _kernel space_ stored at `src` into `dest`. diff --git a/test/integration-ebpf/src/bpf_probe_read.rs b/test/integration-ebpf/src/bpf_probe_read.rs index 013c8b96..02728767 100644 --- a/test/integration-ebpf/src/bpf_probe_read.rs +++ b/test/integration-ebpf/src/bpf_probe_read.rs @@ -12,33 +12,37 @@ const RESULT_BUF_LEN: usize = 1024; macro_rules! read_str_bytes { ($fun:ident, $ptr:expr, $len:expr $(,)?) => { - let r = unsafe { - let Some(ptr) = RESULT.get_ptr_mut(0) else { - return; - }; - &mut *ptr + let Some(ptr) = RESULT.get_ptr_mut(0) else { + return; }; + let TestResult { + did_error, + len, + buf, + } = unsafe { &mut *ptr }; - let s = unsafe { - // $len comes from ctx.arg(1) so it's dynamic and the verifier - // doesn't see any bounds. We do $len.min(RESULT_BUF_LEN) here to - // ensure that the verifier can see the upper bound, or you get: - // - // 18: (79) r7 = *(u64 *)(r7 +8) ; R7_w=scalar() - // [snip] - // 27: (bf) r2 = r7 ; - // R2_w=scalar(id=2,umax=9223372036854775807,var_off=(0x0; 0x7fffffffffffffff)) [snip] - // 28: (85) call bpf_probe_read_user_str#114 - // R2 unbounded memory access, use 'var &= const' or 'if (var < const)' - match $fun($ptr, &mut r.buf[..$len.min(RESULT_BUF_LEN)]) { - Ok(s) => s, - Err(_) => { - r.did_error = 1; - return; - } - } + // $len comes from ctx.arg(1) so it's dynamic and the verifier + // doesn't see any bounds. We do $len.min(RESULT_BUF_LEN) here to + // ensure that the verifier can see the upper bound, or you get: + // + // 18: (79) r7 = *(u64 *)(r7 +8) ; R7_w=scalar() + // [snip] + // 27: (bf) r2 = r7 ; + // R2_w=scalar(id=2,umax=9223372036854775807,var_off=(0x0; 0x7fffffffffffffff)) [snip] + // 28: (85) call bpf_probe_read_user_str#114 + // R2 unbounded memory access, use 'var &= const' or 'if (var < const)' + let Some(buf) = buf.get_mut(..$len) else { + return; }; - r.len = s.len(); + + match unsafe { $fun($ptr, buf) } { + Ok(s) => { + *len = s.len(); + } + Err(_) => { + *did_error = 1; + } + } }; } @@ -53,7 +57,7 @@ struct TestResult { static RESULT: Array = Array::with_max_entries(1, 0); #[map] -static KERNEL_BUFFER: Array<[u8; 1024]> = Array::with_max_entries(1, 0); +static KERNEL_BUFFER: Array<[u8; RESULT_BUF_LEN]> = Array::with_max_entries(1, 0); #[uprobe] pub fn test_bpf_probe_read_user_str_bytes(ctx: ProbeContext) { @@ -85,7 +89,8 @@ pub fn test_bpf_probe_read_kernel_str_bytes(ctx: ProbeContext) { ); } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-ebpf/src/log.rs b/test/integration-ebpf/src/log.rs index e1cc3245..03cdde76 100644 --- a/test/integration-ebpf/src/log.rs +++ b/test/integration-ebpf/src/log.rs @@ -36,7 +36,8 @@ pub fn test_log(ctx: ProbeContext) { } } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-ebpf/src/map_test.rs b/test/integration-ebpf/src/map_test.rs index 87c07557..d63d6fc6 100644 --- a/test/integration-ebpf/src/map_test.rs +++ b/test/integration-ebpf/src/map_test.rs @@ -26,7 +26,8 @@ unsafe fn try_pass(_ctx: XdpContext) -> Result { Ok(xdp_action::XDP_PASS) } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-ebpf/src/name_test.rs b/test/integration-ebpf/src/name_test.rs index f4f1e315..1a5f870c 100644 --- a/test/integration-ebpf/src/name_test.rs +++ b/test/integration-ebpf/src/name_test.rs @@ -15,7 +15,8 @@ unsafe fn try_pass(_ctx: XdpContext) -> Result { Ok(xdp_action::XDP_PASS) } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-ebpf/src/pass.rs b/test/integration-ebpf/src/pass.rs index 8aaf085f..120ef7cc 100644 --- a/test/integration-ebpf/src/pass.rs +++ b/test/integration-ebpf/src/pass.rs @@ -17,7 +17,8 @@ unsafe fn try_pass(_ctx: XdpContext) -> Result { Ok(xdp_action::XDP_PASS) } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-ebpf/src/relocations.rs b/test/integration-ebpf/src/relocations.rs index ee8a21f6..bfbcd1cd 100644 --- a/test/integration-ebpf/src/relocations.rs +++ b/test/integration-ebpf/src/relocations.rs @@ -39,7 +39,8 @@ fn set_result_backward(index: u32, value: u64) { set_result(index, value); } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-ebpf/src/test.rs b/test/integration-ebpf/src/test.rs index 8d40af01..2c4efc91 100644 --- a/test/integration-ebpf/src/test.rs +++ b/test/integration-ebpf/src/test.rs @@ -25,7 +25,8 @@ pub fn test_unload_kpr(_ctx: ProbeContext) -> u32 { 0 } +#[cfg(not(test))] #[panic_handler] fn panic(_info: &core::panic::PanicInfo) -> ! { - unsafe { core::hint::unreachable_unchecked() } + loop {} } diff --git a/test/integration-test/Cargo.toml b/test/integration-test/Cargo.toml index 09770325..a4363549 100644 --- a/test/integration-test/Cargo.toml +++ b/test/integration-test/Cargo.toml @@ -11,7 +11,12 @@ aya-log = { path = "../../aya-log" } aya-obj = { path = "../../aya-obj" } libc = { version = "0.2.105" } log = "0.4" -object = { version = "0.31", default-features = false, features = ["std", "read_core", "elf"] } +matches = "0.1.8" +object = { version = "0.31", default-features = false, features = [ + "std", + "read_core", + "elf", +] } rbpf = "0.2.0" tempfile = "3.3.0" -tokio = { version = "1.24", features = ["rt", "rt-multi-thread", "sync", "time"] } +tokio = { version = "1.24", default-features = false, features = ["time"] } diff --git a/test/integration-test/tests/bpf_probe_read.rs b/test/integration-test/tests/bpf_probe_read.rs index b8d7aedd..d10f82e1 100644 --- a/test/integration-test/tests/bpf_probe_read.rs +++ b/test/integration-test/tests/bpf_probe_read.rs @@ -1,11 +1,4 @@ -use std::process::exit; - -use aya::{ - include_bytes_aligned, - maps::Array, - programs::{ProgramError, UProbe}, - Bpf, -}; +use aya::{include_bytes_aligned, maps::Array, programs::UProbe, Bpf}; const RESULT_BUF_LEN: usize = 1024; @@ -113,16 +106,7 @@ fn load_and_attach_uprobe(prog_name: &str, func_name: &str, bytes: &[u8]) -> Bpf let mut bpf = Bpf::load(bytes).unwrap(); let prog: &mut UProbe = bpf.program_mut(prog_name).unwrap().try_into().unwrap(); - if let Err(ProgramError::LoadError { - io_error, - verifier_log, - }) = prog.load() - { - println!( - "Failed to load program `{prog_name}`: {io_error}. Verifier log:\n{verifier_log:#}" - ); - exit(1); - }; + prog.load().unwrap(); prog.attach(Some(func_name), 0, "/proc/self/exe", None) .unwrap(); diff --git a/test/integration-test/tests/rbpf.rs b/test/integration-test/tests/rbpf.rs index f45ba865..cfeb1334 100644 --- a/test/integration-test/tests/rbpf.rs +++ b/test/integration-test/tests/rbpf.rs @@ -10,10 +10,7 @@ fn run_with_rbpf() { let object = Object::parse(bytes).unwrap(); assert_eq!(object.programs.len(), 1); - assert!(matches!( - object.programs["pass"].section, - ProgramSection::Xdp { .. } - )); + matches::assert_matches!(object.programs["pass"].section, ProgramSection::Xdp { .. }); assert_eq!(object.programs["pass"].section.name(), "pass"); let instructions = &object @@ -42,10 +39,10 @@ fn use_map_with_rbpf() { let mut object = Object::parse(bytes).unwrap(); assert_eq!(object.programs.len(), 1); - assert!(matches!( + matches::assert_matches!( object.programs["tracepoint"].section, ProgramSection::TracePoint { .. } - )); + ); assert_eq!(object.programs["tracepoint"].section.name(), "tracepoint"); // Initialize maps: diff --git a/test/integration-test/tests/relocations.rs b/test/integration-test/tests/relocations.rs index ae95222e..702494c3 100644 --- a/test/integration-test/tests/relocations.rs +++ b/test/integration-test/tests/relocations.rs @@ -1,10 +1,6 @@ -use std::{process::exit, time::Duration}; +use std::time::Duration; -use aya::{ - include_bytes_aligned, - programs::{ProgramError, UProbe}, - Bpf, -}; +use aya::{include_bytes_aligned, programs::UProbe, Bpf}; #[test] fn relocations() { @@ -44,14 +40,7 @@ fn load_and_attach(name: &str, bytes: &[u8]) -> Bpf { let mut bpf = Bpf::load(bytes).unwrap(); let prog: &mut UProbe = bpf.program_mut(name).unwrap().try_into().unwrap(); - if let Err(ProgramError::LoadError { - io_error, - verifier_log, - }) = prog.load() - { - println!("Failed to load program `{name}`: {io_error}. Verifier log:\n{verifier_log:#}"); - exit(1); - }; + prog.load().unwrap(); prog.attach( Some("trigger_relocations_program"), diff --git a/test/run.sh b/test/run.sh index 67056c83..dd2461c7 100755 --- a/test/run.sh +++ b/test/run.sh @@ -249,9 +249,9 @@ exec_vm "rm -rf aya/* libbpf" rsync_vm "--exclude=target --exclude=.tmp $AYA_SOURCE_DIR" rsync_vm "$LIBBPF_DIR" -# need to build or linting will fail trying to include object files -exec_vm "cd aya; cargo xtask build-integration-test --libbpf-dir ~/libbpf" -exec_vm "cd aya; cargo clippy -p integration-test -- --deny warnings" +# need to build or linting will fail trying to include object files; don't run the tests though. +exec_vm "cd aya; cargo xtask integration-test --libbpf-dir ~/libbpf -- filter-that-matches-nothing" +exec_vm "cd aya; cargo clippy --all-targets -p integration-test -- --deny warnings" exec_vm "cd aya; cargo xtask integration-test --libbpf-dir ~/libbpf" # we rm and sync but it doesn't seem to work reliably - I guess we could sleep a diff --git a/xtask/src/build_test.rs b/xtask/src/build_test.rs deleted file mode 100644 index 0fe4c277..00000000 --- a/xtask/src/build_test.rs +++ /dev/null @@ -1,32 +0,0 @@ -use anyhow::Result; -use clap::Parser; -use std::process::Command; - -use crate::{build_ebpf, utils::exec}; - -#[derive(Parser)] -pub struct Options { - /// Target triple for which the code is compiled - #[clap(long)] - pub musl_target: Option, - - #[clap(flatten)] - pub ebpf_options: build_ebpf::BuildEbpfOptions, -} - -pub fn build_test(opts: Options) -> Result<()> { - let Options { - musl_target, - ebpf_options, - } = opts; - - build_ebpf::build_ebpf(ebpf_options)?; - - let mut cmd = Command::new("cargo"); - cmd.args(["build", "-p", "integration-test"]); - - if let Some(target) = musl_target { - cmd.args(["--target", &target]); - } - exec(&mut cmd) -} diff --git a/xtask/src/main.rs b/xtask/src/main.rs index d0a8e333..c86da059 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -1,5 +1,4 @@ mod build_ebpf; -mod build_test; mod codegen; mod docs; mod run; @@ -18,8 +17,6 @@ pub struct XtaskOptions { enum Command { Codegen(codegen::Options), Docs, - BuildIntegrationTest(build_test::Options), - BuildIntegrationTestEbpf(build_ebpf::BuildEbpfOptions), IntegrationTest(run::Options), } @@ -29,8 +26,6 @@ fn main() { let ret = match command { Command::Codegen(opts) => codegen::codegen(opts), Command::Docs => docs::docs(), - Command::BuildIntegrationTest(opts) => build_test::build_test(opts), - Command::BuildIntegrationTestEbpf(opts) => build_ebpf::build_ebpf(opts), Command::IntegrationTest(opts) => run::run(opts), };