From 028433b3a36a63ad117155d3f2f434fa82873abc Mon Sep 17 00:00:00 2001 From: huyubiao Date: Tue, 10 Oct 2023 23:56:45 +0800 Subject: [PATCH 1/2] fix: devmaster startup failure caused by hwdb parameter parsing failure --- exts/devmaster/src/lib/builtin/hwdb.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/exts/devmaster/src/lib/builtin/hwdb.rs b/exts/devmaster/src/lib/builtin/hwdb.rs index 6f641186..63308e37 100644 --- a/exts/devmaster/src/lib/builtin/hwdb.rs +++ b/exts/devmaster/src/lib/builtin/hwdb.rs @@ -39,8 +39,8 @@ struct Args { #[clap(short('p'), long, value_parser)] lookup_prefix: Option, /// - #[clap(short, long, value_parser)] - modalias: Option, + #[clap(required = false, value_parser)] + modalias: Vec, } /// hwdb builtin command @@ -208,7 +208,15 @@ impl Builtin for Hwdb { test: bool, ) -> Result { let dev = exec_unit.get_device(); - let args = Args::parse_from(argv); + let args = match Args::try_parse_from(argv) { + Ok(args) => args, + Err(e) => { + return Err(Error::Other { + msg: format!("Failed to parse argv {:?}", e), + errno: nix::Error::EINVAL, + }) + } + }; if self.hwdb.borrow().is_none() { return Err(Error::Nix { @@ -217,11 +225,11 @@ impl Builtin for Hwdb { } /* query a specific key given as argument */ - if args.modalias.is_some() { + if !args.modalias.is_empty() { match self.lookup( dev, &args.lookup_prefix, - args.modalias.unwrap(), + args.modalias[0].clone(), &args.filter, test, ) { -- Gitee From 25bededda1186d89f325907d1599bb870ac0a2b3 Mon Sep 17 00:00:00 2001 From: huyubiao Date: Fri, 13 Oct 2023 11:04:13 +0800 Subject: [PATCH 2/2] fix: Print error log instead of panic when abnormal, fix trie_fnmatch_f matches the wrong string --- libs/hwdb/src/hwdb_util.rs | 144 +++++++++++++++++++++++++----- libs/hwdb/src/sd_hwdb.rs | 173 +++++++++++++++++++++++-------------- 2 files changed, 227 insertions(+), 90 deletions(-) diff --git a/libs/hwdb/src/hwdb_util.rs b/libs/hwdb/src/hwdb_util.rs index 47f9d37d..2ff6243a 100644 --- a/libs/hwdb/src/hwdb_util.rs +++ b/libs/hwdb/src/hwdb_util.rs @@ -48,7 +48,10 @@ impl Trie { let f = match File::open(filename) { Ok(f) => f, - Err(e) => return Err(Errno::from_i32(e.raw_os_error().unwrap())), + Err(e) => { + log::error!("Failed to open file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); + } }; let mut line_number = 0; @@ -64,7 +67,10 @@ impl Trie { break; } } - Err(e) => return Err(Errno::from_i32(e.raw_os_error().unwrap())), + Err(e) => { + log::error!("Failed to read line:{:?} err:{:?}", line, e); + return Err(Errno::EINVAL); + } } line_number += 1; @@ -352,15 +358,25 @@ impl Trie { t.store_nodes_size(&self.root, compat); - let mut f = match File::create(filename) { + let mut f = match File::create(&filename) { Ok(f) => f, - Err(e) => return Err(Errno::from_i32(e.raw_os_error().unwrap())), + Err(e) => { + log::error!("Failed to create file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); + } }; - let mut permissions = f.metadata().unwrap().permissions(); + let mut permissions = match f.metadata() { + Ok(data) => data.permissions(), + Err(e) => { + log::error!("Failed to get metadata err:{:?}", e); + return Err(Errno::EINVAL); + } + }; permissions.set_mode(0o444); if let Err(e) = f.set_permissions(permissions) { - return Err(Errno::from_i32(e.raw_os_error().unwrap())); + log::error!("Failed to set permissions err:{:?}", e); + return Err(Errno::EINVAL); } let header_size = usize::to_le(size_of::()); @@ -382,29 +398,56 @@ impl Trie { /* write nodes */ if let Err(e) = f.seek(SeekFrom::Start(size_of::() as u64)) { - return Err(Errno::from_i32(e.raw_os_error().unwrap())); + log::error!("Failed to seek file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); } let root_off = t.store_nodes(&mut f, &self.root, compat)?; h.set_nodes_root_off(usize::to_le(root_off)); - let pos = f.seek(SeekFrom::Current(0)).unwrap() as usize; + let pos = match f.seek(SeekFrom::Current(0)) { + Ok(pos) => pos as usize, + Err(e) => { + log::error!("Failed to seek file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); + } + }; h.set_nodes_len(usize::to_le(pos - size_of::())); /* write string buffer */ - f.write_all(&self.strings.buf).unwrap(); + if let Err(e) = f.write_all(&self.strings.buf) { + log::error!("Failed to write_all file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); + } h.set_strings_len(usize::to_le(self.strings.buf.len())); /* write header */ - let size = f.seek(SeekFrom::Current(0)).unwrap() as usize; + let size = match f.seek(SeekFrom::Current(0)) { + Ok(size) => size as usize, + Err(e) => { + log::error!("Failed to seek file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); + } + }; h.set_file_size(usize::to_le(size)); if let Err(e) = f.seek(SeekFrom::Start(0)) { - return Err(Errno::from_i32(e.raw_os_error().unwrap())); + log::error!("Failed to seek file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); } - let encoded: Vec = bincode::serialize(&h).unwrap(); - f.write_all(encoded.as_slice()).unwrap(); + let encoded = match bincode::serialize(&h) { + Ok(encoded) => encoded, + Err(e) => { + log::error!("Failed to serialize TrieHeaderF err:{:?}", e); + return Err(Errno::EINVAL); + } + }; + + if let Err(e) = f.write_all(encoded.as_slice()) { + log::error!("Failed to write_all file:{:?} err:{:?}", filename, e); + return Err(Errno::EINVAL); + } /* write succeeded */ log::debug!("=== trie on-disk ==="); @@ -495,16 +538,42 @@ impl TrieF { ); /* write node */ - let node_off = f.seek(SeekFrom::Current(0)).unwrap() as usize; + let node_off = match f.seek(SeekFrom::Current(0)) { + Ok(off) => off as usize, + Err(e) => { + log::error!("Failed to seek file err:{:?}", e); + return Err(Errno::EINVAL); + } + }; - let encoded: Vec = bincode::serialize(&n).unwrap(); - f.write_all(encoded.as_slice()).unwrap(); + let encoded = match bincode::serialize(&n) { + Ok(encoded) => encoded, + Err(e) => { + log::error!("Failed to serialize TrieNodeF err:{:?}", e); + return Err(Errno::EINVAL); + } + }; + + if let Err(e) = f.write_all(encoded.as_slice()) { + log::error!("Failed to write_all file err:{:?}", e); + return Err(Errno::EINVAL); + } self.nodes_count += 1; /* append children array */ for child in children.iter().take(node.borrow().children.len()) { - let encoded: Vec = bincode::serialize(&child).unwrap(); - f.write_all(encoded.as_slice()).unwrap(); + let encoded = match bincode::serialize(&child) { + Ok(encoded) => encoded, + Err(e) => { + log::error!("Failed to serialize TrieChildEntryF err:{:?}", e); + return Err(Errno::EINVAL); + } + }; + + if let Err(e) = f.write_all(encoded.as_slice()) { + log::error!("Failed to write_all file err:{:?}", e); + return Err(Errno::EINVAL); + } } self.children_count += node.borrow().children.len(); @@ -514,8 +583,18 @@ impl TrieF { let value_off = usize::to_le(self.strings_off + node.borrow().values[i].value_off); if compat { let v = TrieValueEntryF::new(key_off, value_off); - let encoded: Vec = bincode::serialize(&v).unwrap(); - f.write_all(encoded.as_slice()).unwrap(); + let encoded = match bincode::serialize(&v) { + Ok(encoded) => encoded, + Err(e) => { + log::error!("Failed to serialize TrieValueEntryF err:{:?}", e); + return Err(Errno::EINVAL); + } + }; + + if let Err(e) = f.write_all(encoded.as_slice()) { + log::error!("Failed to write_all file err:{:?}", e); + return Err(Errno::EINVAL); + } } else { let filename_off = usize::to_le(self.strings_off + node.borrow().values[i].filename_off); @@ -528,8 +607,18 @@ impl TrieF { line_number, file_priority, ); - let encoded: Vec = bincode::serialize(&v).unwrap(); - f.write_all(encoded.as_slice()).unwrap(); + let encoded = match bincode::serialize(&v) { + Ok(encoded) => encoded, + Err(e) => { + log::error!("Failed to serialize TrieValueEntry2F err:{:?}", e); + return Err(Errno::EINVAL); + } + }; + + if let Err(e) = f.write_all(encoded.as_slice()) { + log::error!("Failed to write_all file err:{:?}", e); + return Err(Errno::EINVAL); + } }; } self.values_count += node.borrow().values.len(); @@ -711,8 +800,15 @@ impl HwdbUtil { log::debug!("strings: {:?} bytes", trie.strings.buf.len()); let permissions = Permissions::from_mode(0o755); - create_dir_all(&bin_dir).unwrap(); - std::fs::set_permissions(&bin_dir, permissions).unwrap(); + if let Err(e) = create_dir_all(&bin_dir) { + log::error!("Failed to create dir:{:?} err:{:?}", bin_dir, e); + return Err(Errno::EINVAL); + } + + if let Err(e) = std::fs::set_permissions(&bin_dir, permissions) { + log::error!("Failed to set permissions dir:{:?} err:{:?}", bin_dir, e); + return Err(Errno::EINVAL); + } trie.store(hwdb_bin, compat)?; diff --git a/libs/hwdb/src/sd_hwdb.rs b/libs/hwdb/src/sd_hwdb.rs index b4741c76..692e5307 100644 --- a/libs/hwdb/src/sd_hwdb.rs +++ b/libs/hwdb/src/sd_hwdb.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs::File; use std::fs::OpenOptions; -use std::io::{Read, Seek}; +use std::io::Read; use std::os::unix::prelude::AsRawFd; use std::time::{Duration, UNIX_EPOCH}; @@ -283,7 +283,7 @@ impl SdHwdb { None => return Err(Errno::ENOENT), }; - let value = self.trie_string(entry.value_off); + let value = self.trie_string(entry.value_off)?; Ok(value) } @@ -313,7 +313,7 @@ impl SdHwdb { for it in self.properties.iter() { let key = it.0; let entry = it.1; - let value = self.trie_string(entry.value_off); + let value = self.trie_string(entry.value_off)?; map.insert(key.to_string(), value); } @@ -329,13 +329,13 @@ impl SdHwdb { fn trie_search_f(&mut self, search: String) -> Result<()> { let mut i: usize = 0; let mut buf = LineBuf::new(); - let mut node = self.trie_node_from_off(self.head.nodes_root_off); + let mut node = self.trie_node_from_off(self.head.nodes_root_off)?; loop { let mut p: usize = 0; if 0 != node.trie_node_f.prefix_off { loop { - let s = self.trie_string(node.trie_node_f.prefix_off); + let s = self.trie_string(node.trie_node_f.prefix_off)?; if s.len() == p { break; } @@ -400,11 +400,10 @@ impl SdHwdb { return Ok(()); } - let child = self.node_lookup_f(&node, search.as_bytes()[i]); - if child.is_none() { - break; + match self.node_lookup_f(&node, search.as_bytes()[i]) { + Some(child) => node = child, + None => break, } - node = child.unwrap(); i += 1; } Ok(()) @@ -418,13 +417,22 @@ impl SdHwdb { let off = node.node_index + usize::from_le(self.head.node_size) + child_count as usize * usize::from_le(self.head.child_entry_size); - let child: TrieChildEntryF = bincode::deserialize(&self.map[off..]).unwrap(); + let child = match bincode::deserialize::(&self.map[off..]) { + Ok(child) => child, + Err(e) => { + log::error!("Failed to lookup child err:{:?}", e); + continue; + } + }; children.push(child); } children.sort_by_key(|search| search.c); match children.binary_search_by_key(&search.c, |search| search.c) { - Ok(a) => Some(self.trie_node_from_off(children[a].child_off)), + Ok(index) => match self.trie_node_from_off(children[index].child_off) { + Ok(trie_node) => Some(trie_node), + Err(_) => None, + }, Err(_) => None, } } @@ -436,26 +444,36 @@ impl SdHwdb { buf: &mut LineBuf, search: &str, ) -> Result<()> { - let prefix = self.trie_string(node.trie_node_f.prefix_off); + let prefix = self.trie_string(node.trie_node_f.prefix_off)?; let add_prefix = prefix[p..].to_string(); buf.add(&add_prefix); let len = add_prefix.len(); for i in 0..node.trie_node_f.children_count { - let child = self.trie_node_child(node.clone(), i as usize); + let child = self.trie_node_child(node.clone(), i as usize)?; buf.add_char(child.c); - let f = self.trie_node_from_off(child.child_off); + let f = self.trie_node_from_off(child.child_off)?; if let Err(e) = self.trie_fnmatch_f(f, 0, buf, search) { return Err(e); } buf.rem_char(); } - let pattern = Pattern::new(&buf.get()).unwrap(); - if usize::from_le(node.trie_node_f.values_count) > 0 && pattern.matches(search) { - for i in 0..usize::from_le(node.trie_node_f.values_count) { - if let Err(e) = self.add_property(&node, i) { - return Err(e); + if usize::from_le(node.trie_node_f.values_count) > 0 { + match Pattern::new(&buf.get()) { + Ok(pattern) => { + if pattern.matches(search) { + for i in 0..usize::from_le(node.trie_node_f.values_count) { + if let Err(e) = self.add_property(&node, i) { + return Err(e); + } + } + } else { + log::error!("Pattern error buf:{:?}", buf.get()); + } + } + Err(e) => { + log::error!("Failed to new Pattern err:{:?} buf:{:?}", e, buf.get()); } } } @@ -464,29 +482,39 @@ impl SdHwdb { Ok(()) } - fn trie_node_value(&self, node: &TrieNode, idx: usize) -> TrieValueEntryF { + fn trie_node_value(&self, node: &TrieNode, idx: usize) -> Result { let mut off = node.node_index + usize::from_le(self.head.node_size); off += node.trie_node_f.children_count as usize * usize::from_le(self.head.child_entry_size); off += idx * usize::from_le(self.head.value_entry_size); - let value: TrieValueEntryF = bincode::deserialize(&self.map[off..]).unwrap(); - value + match bincode::deserialize::(&self.map[off..]) { + Ok(value) => Ok(value), + Err(e) => { + log::error!("Failed to deserialize node_value err:{:?}", e); + Err(Errno::EINVAL) + } + } } - fn trie_node_value2(&self, node: &TrieNode, idx: usize) -> TrieValueEntry2F { + fn trie_node_value2(&self, node: &TrieNode, idx: usize) -> Result { let mut off = node.node_index + usize::from_le(self.head.node_size); off += node.trie_node_f.children_count as usize * usize::from_le(self.head.child_entry_size); off += idx * usize::from_le(self.head.value_entry_size); - let value: TrieValueEntry2F = bincode::deserialize(&self.map[off..]).unwrap(); - value + match bincode::deserialize::(&self.map[off..]) { + Ok(value) => Ok(value), + Err(e) => { + log::error!("Failed to deserialize node_value2 err:{:?}", e); + Err(Errno::EINVAL) + } + } } fn add_property(&mut self, node: &TrieNode, idx: usize) -> Result<()> { - let entry = self.trie_node_value(node, idx); - let mut key = self.trie_string(entry.key_off); + let entry = self.trie_node_value(node, idx)?; + let mut key = self.trie_string(entry.key_off)?; let mut entry2 = TrieValueEntry2F::new(entry.key_off, entry.value_off, 0, 0, 0); @@ -501,7 +529,7 @@ impl SdHwdb { key.remove(0); if usize::from_le(self.head.value_entry_size) >= std::mem::size_of::() { - entry2 = self.trie_node_value2(node, idx); + entry2 = self.trie_node_value2(node, idx)?; if let Some(old) = self.properties.get(&key) { /* On duplicates, we order by filename priority and line-number. * @@ -548,23 +576,34 @@ impl SdHwdb { Ok(()) } - fn trie_node_child(&self, node: TrieNode, idx: usize) -> TrieChildEntryF { + fn trie_node_child(&self, node: TrieNode, idx: usize) -> Result { let off = node.node_index + usize::from_le(self.head.node_size) + idx * usize::from_le(self.head.child_entry_size); - let child: TrieChildEntryF = bincode::deserialize(&self.map[off..]).unwrap(); - child + match bincode::deserialize::(&self.map[off..]) { + Ok(child) => Ok(child), + Err(e) => { + log::error!("Failed to deserialize TrieChildEntryF err:{:?}", e); + Err(Errno::EINVAL) + } + } } - fn trie_node_from_off(&mut self, off: usize) -> TrieNode { + fn trie_node_from_off(&mut self, off: usize) -> Result { let trie_node_off = usize::from_le(off); - let trie_node_f: TrieNodeF = bincode::deserialize(&self.map[trie_node_off..]).unwrap(); + let trie_node_f = match bincode::deserialize::(&self.map[trie_node_off..]) { + Ok(trie_node_f) => trie_node_f, + Err(e) => { + log::error!("Failed to deserialize TrieNodeF err:{:?}", e); + return Err(Errno::EINVAL); + } + }; - TrieNode::new(trie_node_f, trie_node_off) + Ok(TrieNode::new(trie_node_f, trie_node_off)) } - fn trie_string(&self, off: usize) -> String { + fn trie_string(&self, off: usize) -> Result { let mut s: Vec = Vec::new(); let mut i = 0; while let Ok(c) = bincode::deserialize::(&self.map[usize::from_le(off) + i..]) { @@ -575,14 +614,12 @@ impl SdHwdb { } i += 1; } - String::from_utf8(s).unwrap() - } -} - -impl Drop for SdHwdb { - fn drop(&mut self) { - if nix::unistd::close(self.f.as_raw_fd()).is_err() { - log::error!("Failed to close fd {:?}", self.f); + match String::from_utf8(s) { + Ok(trie_str) => Ok(trie_str), + Err(e) => { + log::error!("Failed to from_utf8 err:{:?}", e); + Err(Errno::EINVAL) + } } } } @@ -598,26 +635,28 @@ fn hwdb_new(path: &str) -> Result { file = match OpenOptions::new().read(true).open(path) { Ok(f) => Some(f), Err(e) => { - log::error!("Failed to open {:?}", path); - return Err(Errno::from_i32(e.raw_os_error().unwrap())); + log::error!("Failed to open {:?} err:{:?}", path, e); + return Err(Errno::EINVAL); } }; } else { for p in HWDB_BIN_PATHS { log::debug!("Trying to open \"{:?}\"...", p); - let f = OpenOptions::new().read(true).open(p); - if let Ok(ff) = f { - file = Some(ff); - hwdb_path = p; - break; - } - - let err = Errno::from_i32(f.err().unwrap().raw_os_error().unwrap()); - if err != Errno::ENOENT { - log::error!("Failed to open {:?}", p); - return Err(err); + match OpenOptions::new().read(true).open(p) { + Ok(f) => { + file = Some(f); + hwdb_path = p; + break; + } + Err(e) => { + if e.kind() != std::io::ErrorKind::NotFound { + log::error!("Failed to open {:?} err:{:?}", p, e); + return Err(Errno::EINVAL); + } + } } } + if file.is_none() { log::error!("hwdb.bin does not exist, please run 'sysmaster-hwdb update'"); return Err(Errno::ENOENT); @@ -642,18 +681,20 @@ fn hwdb_new(path: &str) -> Result { return Err(Errno::EFBIG); } - hwdb_file.seek(std::io::SeekFrom::Start(0)).unwrap(); - let mut buffer: [u8; 1024] = [0; 1024]; - let mut hwdb_map = Vec::new(); - loop { - let n = hwdb_file.read(&mut buffer).unwrap(); - if 0 == n { - break; - } - hwdb_map.extend_from_slice(&buffer[..n]); + let mut hwdb_map = Vec::with_capacity(hwdb_st.st_size as usize); + if let Err(e) = hwdb_file.read_to_end(&mut hwdb_map) { + log::error!("Failed to read_to_end of {:?} err:{:?}", hwdb_path, e); + return Err(Errno::EINVAL); } - let hwdb_head: TrieHeaderF = bincode::deserialize(&hwdb_map).unwrap(); + let hwdb_head = match bincode::deserialize::(&hwdb_map) { + Ok(hwdb_head) => hwdb_head, + Err(e) => { + log::error!("Failed to deserialize TrieHeaderF err:{:?}", e); + return Err(Errno::EINVAL); + } + }; + if hwdb_head.signature != sig || hwdb_st.st_size as usize != usize::from_le(hwdb_head.file_size) { log::error!("Failed to recognize the format of {:?}", hwdb_path); -- Gitee