From b8d1271bc07dc4cefc911ca1e9f3fd7348481403 Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Fri, 17 Sep 2021 11:55:33 +0800 Subject: [PATCH 1/7] Pass PacketBuf as an argument of API --- examples/armv4t/gdb/host_io.rs | 57 ++++++++++--------- examples/armv4t/gdb/memory_map.rs | 18 +++++- .../gdb/target_description_xml_override.rs | 18 +++++- src/gdbstub_impl/ext/base.rs | 33 +++++++---- src/gdbstub_impl/ext/host_io.rs | 48 ++++------------ src/gdbstub_impl/ext/memory_map.rs | 12 +++- src/protocol/commands.rs | 6 +- src/protocol/commands/_qXfer_features_read.rs | 19 ++++--- src/protocol/commands/_qXfer_memory_map.rs | 19 ++++--- src/protocol/commands/_vFile_pread.rs | 19 +++++-- src/protocol/commands/_vFile_readlink.rs | 8 ++- src/protocol/response_writer.rs | 19 ------- src/target/ext/host_io.rs | 38 ++----------- src/target/ext/memory_map.rs | 9 ++- .../ext/target_description_xml_override.rs | 9 ++- 15 files changed, 170 insertions(+), 162 deletions(-) diff --git a/examples/armv4t/gdb/host_io.rs b/examples/armv4t/gdb/host_io.rs index a3a6e3f1..74f2703a 100644 --- a/examples/armv4t/gdb/host_io.rs +++ b/examples/armv4t/gdb/host_io.rs @@ -2,8 +2,7 @@ use std::io::{Read, Seek, Write}; use gdbstub::target; use gdbstub::target::ext::host_io::{ - FsKind, HostIoErrno, HostIoError, HostIoOpenFlags, HostIoOpenMode, HostIoOutput, HostIoResult, - HostIoStat, HostIoToken, + FsKind, HostIoErrno, HostIoError, HostIoOpenFlags, HostIoOpenMode, HostIoResult, HostIoStat, }; use crate::emu::Emu; @@ -133,16 +132,18 @@ impl target::ext::host_io::HostIoPread for Emu { fn pread<'a>( &mut self, fd: u32, - count: u32, - offset: u32, - output: HostIoOutput<'a>, - ) -> HostIoResult, Self> { + count: usize, + offset: u64, + buf: &mut [u8], + ) -> HostIoResult { if fd < FD_RESERVED { if fd == 0 { let len = TEST_PROGRAM_ELF.len(); - return Ok(output.write( - &TEST_PROGRAM_ELF[len.min(offset as usize)..len.min((offset + count) as usize)], - )); + let data = + &TEST_PROGRAM_ELF[len.min(offset as usize)..len.min(offset as usize + count)]; + let buf = &mut buf[..data.len()]; + buf.copy_from_slice(data); + return Ok(data.len()); } else { return Err(HostIoError::Errno(HostIoErrno::EBADF)); } @@ -153,10 +154,9 @@ impl target::ext::host_io::HostIoPread for Emu { _ => return Err(HostIoError::Errno(HostIoErrno::EBADF)), }; - let mut buffer = vec![0; count as usize]; - file.seek(std::io::SeekFrom::Start(offset as u64))?; - let n = file.read(&mut buffer)?; - Ok(output.write(&buffer[..n])) + file.seek(std::io::SeekFrom::Start(offset))?; + let n = file.read(buf)?; + Ok(n) } } @@ -246,29 +246,34 @@ impl target::ext::host_io::HostIoUnlink for Emu { } impl target::ext::host_io::HostIoReadlink for Emu { - fn readlink<'a>( - &mut self, - filename: &[u8], - output: HostIoOutput<'a>, - ) -> HostIoResult, Self> { + fn readlink<'a>(&mut self, filename: &[u8], buf: &mut [u8]) -> HostIoResult { if filename == b"/proc/1/exe" { // Support `info proc exe` command - return Ok(output.write(b"/test.elf")); + let exe = b"/test.elf"; + let buf = &mut buf[..exe.len()]; + buf.copy_from_slice(exe); + return Ok(exe.len()); } else if filename == b"/proc/1/cwd" { // Support `info proc cwd` command - return Ok(output.write(b"/")); + let cwd = b"/"; + let buf = &mut buf[..cwd.len()]; + buf.copy_from_slice(cwd); + return Ok(cwd.len()); } else if filename.starts_with(b"/proc") { return Err(HostIoError::Errno(HostIoErrno::ENOENT)); } let path = std::str::from_utf8(filename).map_err(|_| HostIoError::Errno(HostIoErrno::ENOENT))?; - Ok(output.write( - std::fs::read_link(path)? - .to_str() - .ok_or(HostIoError::Errno(HostIoErrno::ENOENT))? - .as_bytes(), - )) + let link = std::fs::read_link(path)?; + let data = link + .to_str() + .ok_or(HostIoError::Errno(HostIoErrno::ENOENT))? + .as_bytes(); + let len = data.len(); + let buf = &mut buf[..len]; + buf.copy_from_slice(data); + Ok(len) } } diff --git a/examples/armv4t/gdb/memory_map.rs b/examples/armv4t/gdb/memory_map.rs index a9e29a9e..82d26a1d 100644 --- a/examples/armv4t/gdb/memory_map.rs +++ b/examples/armv4t/gdb/memory_map.rs @@ -1,17 +1,31 @@ use gdbstub::target; +use gdbstub::target::TargetResult; use crate::emu::Emu; impl target::ext::memory_map::MemoryMap for Emu { - fn memory_map_xml(&self) -> &str { + fn memory_map_xml( + &self, + offset: u64, + length: usize, + buf: &mut [u8], + ) -> TargetResult { // Sample memory map, with RAM coverying the whole // memory space. - r#" + let memory_map = r#" "# + .trim() + .as_bytes(); + + let len = memory_map.len(); + let data = &memory_map[len.min(offset as usize)..len.min(offset as usize + length)]; + let buf = &mut buf[..data.len()]; + buf.copy_from_slice(data); + Ok(data.len()) } } diff --git a/examples/armv4t/gdb/target_description_xml_override.rs b/examples/armv4t/gdb/target_description_xml_override.rs index 24801310..4fc4d139 100644 --- a/examples/armv4t/gdb/target_description_xml_override.rs +++ b/examples/armv4t/gdb/target_description_xml_override.rs @@ -1,10 +1,16 @@ use gdbstub::target; +use gdbstub::target::TargetResult; use crate::emu::Emu; impl target::ext::target_description_xml_override::TargetDescriptionXmlOverride for Emu { - fn target_description_xml(&self) -> &str { - r#" + fn target_description_xml( + &self, + offset: u64, + length: usize, + buf: &mut [u8], + ) -> TargetResult { + let xml = r#" armv4t @@ -67,5 +73,13 @@ impl target::ext::target_description_xml_override::TargetDescriptionXmlOverride "# + .trim() + .as_bytes(); + + let len = xml.len(); + let data = &xml[len.min(offset as usize)..len.min(offset as usize + length)]; + let buf = &mut buf[..data.len()]; + buf.copy_from_slice(data); + Ok(data.len()) } } diff --git a/src/gdbstub_impl/ext/base.rs b/src/gdbstub_impl/ext/base.rs index cfbfe678..1f5825a7 100644 --- a/src/gdbstub_impl/ext/base.rs +++ b/src/gdbstub_impl/ext/base.rs @@ -127,20 +127,29 @@ impl GdbStubImpl { } Base::qXferFeaturesRead(cmd) => { #[allow(clippy::redundant_closure)] - let xml = target - .target_description_xml_override() - .map(|ops| ops.target_description_xml()) - .or_else(|| T::Arch::target_description_xml()); - - match xml { - Some(xml) => { + let ret = if let Some(ops) = target.target_description_xml_override() { + ops.target_description_xml(cmd.offset, cmd.length, cmd.buf) + .handle_error()? + } else { + if let Some(xml) = T::Arch::target_description_xml() { let xml = xml.trim().as_bytes(); - res.write_binary_range(xml, cmd.offset, cmd.len)?; + let len = xml.len(); + let data = &xml[len.min(cmd.offset as usize) + ..len.min(cmd.offset as usize + cmd.length)]; + let buf = &mut cmd.buf[..data.len()]; + buf.copy_from_slice(data); + data.len() + } else { + 0 } - // If the target hasn't provided their own XML, then the initial response to - // "qSupported" wouldn't have included "qXfer:features:read", and gdb wouldn't - // send this packet unless it was explicitly marked as supported. - None => return Err(Error::PacketUnexpected), + }; + + if ret == 0 { + res.write_str("l")?; + } else { + res.write_str("m")?; + // TODO: add more specific error variant? + res.write_binary(cmd.buf.get(..ret).ok_or(Error::PacketBufferOverflow)?)?; } HandlerStatus::Handled } diff --git a/src/gdbstub_impl/ext/host_io.rs b/src/gdbstub_impl/ext/host_io.rs index 911decda..f9afa647 100644 --- a/src/gdbstub_impl/ext/host_io.rs +++ b/src/gdbstub_impl/ext/host_io.rs @@ -2,7 +2,7 @@ use super::prelude::*; use crate::protocol::commands::ext::HostIo; use crate::arch::Arch; -use crate::target::ext::host_io::{HostIoError, HostIoOutput, HostIoStat}; +use crate::target::ext::host_io::{HostIoError, HostIoStat}; impl GdbStubImpl { pub(crate) fn handle_host_io( @@ -52,31 +52,16 @@ impl GdbStubImpl { HandlerStatus::Handled } HostIo::vFilePread(cmd) if ops.enable_pread().is_some() => { - let count = ::Usize::from_be_bytes(cmd.count) - .ok_or(Error::TargetMismatch)?; - let offset = ::Usize::from_be_bytes(cmd.offset) - .ok_or(Error::TargetMismatch)?; - let mut err: Result<_, Error> = Ok(()); - let mut callback = |data: &[u8]| { - let e = (|| { + let ops = ops.enable_pread().unwrap(); + handle_hostio_result! { + if let Ok(ret) = ops.pread(cmd.fd, cmd.count, cmd.offset, cmd.buf) => { res.write_str("F")?; - res.write_num(data.len())?; + res.write_num(ret)?; res.write_str(";")?; - res.write_binary(data)?; - Ok(()) - })(); - - if let Err(e) = e { - err = Err(e) + res.write_binary(cmd.buf.get(..ret).ok_or(Error::PacketBufferOverflow)?)?; } }; - let ops = ops.enable_pread().unwrap(); - handle_hostio_result! { - if let Ok(_) = ops.pread(cmd.fd, count, offset, HostIoOutput::new(&mut callback)) => {} - }; - err?; - HandlerStatus::Handled } HostIo::vFilePwrite(cmd) if ops.enable_pwrite().is_some() => { @@ -126,27 +111,16 @@ impl GdbStubImpl { HandlerStatus::Handled } HostIo::vFileReadlink(cmd) if ops.enable_readlink().is_some() => { - let mut err: Result<_, Error> = Ok(()); - let mut callback = |data: &[u8]| { - let e = (|| { + let ops = ops.enable_readlink().unwrap(); + handle_hostio_result! { + if let Ok(ret) = ops.readlink(cmd.filename, cmd.buf) => { res.write_str("F")?; - res.write_num(data.len())?; + res.write_num(ret)?; res.write_str(";")?; - res.write_binary(data)?; - Ok(()) - })(); - - if let Err(e) = e { - err = Err(e) + res.write_binary(cmd.buf.get(..ret).ok_or(Error::PacketBufferOverflow)?)?; } }; - let ops = ops.enable_readlink().unwrap(); - handle_hostio_result! { - if let Ok(_) = ops.readlink(cmd.filename, HostIoOutput::new(&mut callback)) => {} - }; - err?; - HandlerStatus::Handled } HostIo::vFileSetfs(cmd) if ops.enable_setfs().is_some() => { diff --git a/src/gdbstub_impl/ext/memory_map.rs b/src/gdbstub_impl/ext/memory_map.rs index 7003ca7f..cb622b93 100644 --- a/src/gdbstub_impl/ext/memory_map.rs +++ b/src/gdbstub_impl/ext/memory_map.rs @@ -17,8 +17,16 @@ impl GdbStubImpl { let handler_status = match command { MemoryMap::qXferMemoryMapRead(cmd) => { - let xml = ops.memory_map_xml().trim().as_bytes(); - res.write_binary_range(xml, cmd.offset, cmd.len)?; + let ret = ops + .memory_map_xml(cmd.offset, cmd.length, cmd.buf) + .handle_error()?; + if ret == 0 { + res.write_str("l")?; + } else { + res.write_str("m")?; + // TODO: add more specific error variant? + res.write_binary(cmd.buf.get(..ret).ok_or(Error::PacketBufferOverflow)?)?; + } HandlerStatus::Handled } }; diff --git a/src/protocol/commands.rs b/src/protocol/commands.rs index b171ea6e..4500af87 100644 --- a/src/protocol/commands.rs +++ b/src/protocol/commands.rs @@ -180,7 +180,7 @@ commands! { "QStartNoAckMode" => _QStartNoAckMode::QStartNoAckMode, "qsThreadInfo" => _qsThreadInfo::qsThreadInfo, "qSupported" => _qSupported::qSupported<'a>, - "qXfer:features:read" => _qXfer_features_read::qXferFeaturesRead, + "qXfer:features:read" => _qXfer_features_read::qXferFeaturesRead<'a>, "s" => _s::s<'a>, "T" => _t_upcase::T, "vCont" => _vCont::vCont<'a>, @@ -221,8 +221,8 @@ commands! { "bs" => _bs::bs, } - memory_map { - "qXfer:memory-map:read" => _qXfer_memory_map::qXferMemoryMapRead, + memory_map use 'a { + "qXfer:memory-map:read" => _qXfer_memory_map::qXferMemoryMapRead<'a>, } exec_file use 'a { diff --git a/src/protocol/commands/_qXfer_features_read.rs b/src/protocol/commands/_qXfer_features_read.rs index 6e6d3777..cc89544e 100644 --- a/src/protocol/commands/_qXfer_features_read.rs +++ b/src/protocol/commands/_qXfer_features_read.rs @@ -1,14 +1,17 @@ use super::prelude::*; #[derive(Debug)] -pub struct qXferFeaturesRead { - pub offset: usize, - pub len: usize, +pub struct qXferFeaturesRead<'a> { + pub offset: u64, + pub length: usize, + + pub buf: &'a mut [u8], } -impl<'a> ParseCommand<'a> for qXferFeaturesRead { +impl<'a> ParseCommand<'a> for qXferFeaturesRead<'a> { fn from_packet(buf: PacketBuf<'a>) -> Option { - let body = buf.into_body(); + let (buf, body_range) = buf.into_raw_buf(); + let body = buf.get_mut(body_range.start..body_range.end)?; if body.is_empty() { return None; @@ -22,8 +25,10 @@ impl<'a> ParseCommand<'a> for qXferFeaturesRead { let mut body = body.next()?.split(|b| *b == b','); let offset = decode_hex(body.next()?).ok()?; - let len = decode_hex(body.next()?).ok()?; + let length = decode_hex(body.next()?).ok()?; + + drop(body); - Some(qXferFeaturesRead { offset, len }) + Some(qXferFeaturesRead { offset, length, buf }) } } diff --git a/src/protocol/commands/_qXfer_memory_map.rs b/src/protocol/commands/_qXfer_memory_map.rs index e522de09..df7ed055 100644 --- a/src/protocol/commands/_qXfer_memory_map.rs +++ b/src/protocol/commands/_qXfer_memory_map.rs @@ -1,14 +1,17 @@ use super::prelude::*; #[derive(Debug)] -pub struct qXferMemoryMapRead { - pub offset: usize, - pub len: usize, +pub struct qXferMemoryMapRead<'a> { + pub offset: u64, + pub length: usize, + + pub buf: &'a mut [u8], } -impl<'a> ParseCommand<'a> for qXferMemoryMapRead { +impl<'a> ParseCommand<'a> for qXferMemoryMapRead<'a> { fn from_packet(buf: PacketBuf<'a>) -> Option { - let body = buf.into_body(); + let (buf, body_range) = buf.into_raw_buf(); + let body = buf.get_mut(body_range.start..body_range.end)?; if body.is_empty() { return None; @@ -22,8 +25,10 @@ impl<'a> ParseCommand<'a> for qXferMemoryMapRead { let mut body = body.next()?.split(|b| *b == b','); let offset = decode_hex(body.next()?).ok()?; - let len = decode_hex(body.next()?).ok()?; + let length = decode_hex(body.next()?).ok()?; + + drop(body); - Some(qXferMemoryMapRead { offset, len }) + Some(qXferMemoryMapRead { offset, length , buf}) } } diff --git a/src/protocol/commands/_vFile_pread.rs b/src/protocol/commands/_vFile_pread.rs index 4b795952..b2318297 100644 --- a/src/protocol/commands/_vFile_pread.rs +++ b/src/protocol/commands/_vFile_pread.rs @@ -3,13 +3,17 @@ use super::prelude::*; #[derive(Debug)] pub struct vFilePread<'a> { pub fd: u32, - pub count: &'a [u8], - pub offset: &'a [u8], + pub count: usize, + pub offset: u64, + + pub buf: &'a mut [u8], } impl<'a> ParseCommand<'a> for vFilePread<'a> { fn from_packet(buf: PacketBuf<'a>) -> Option { - let body = buf.into_body(); + let (buf, body_range) = buf.into_raw_buf(); + let body = buf.get_mut(body_range.start..body_range.end)?; + if body.is_empty() { return None; } @@ -18,9 +22,12 @@ impl<'a> ParseCommand<'a> for vFilePread<'a> { [b':', body @ ..] => { let mut body = body.splitn_mut_no_panic(3, |b| *b == b','); let fd = decode_hex(body.next()?).ok()?; - let count = decode_hex_buf(body.next()?).ok()?; - let offset = decode_hex_buf(body.next()?).ok()?; - Some(vFilePread{fd, count, offset}) + let count = decode_hex(body.next()?).ok()?; + let offset = decode_hex(body.next()?).ok()?; + + drop(body); + + Some(vFilePread{fd, count, offset, buf}) }, _ => None, } diff --git a/src/protocol/commands/_vFile_readlink.rs b/src/protocol/commands/_vFile_readlink.rs index 1223bed5..b56201fd 100644 --- a/src/protocol/commands/_vFile_readlink.rs +++ b/src/protocol/commands/_vFile_readlink.rs @@ -3,11 +3,15 @@ use super::prelude::*; #[derive(Debug)] pub struct vFileReadlink<'a> { pub filename: &'a [u8], + + pub buf: &'a mut [u8], } impl<'a> ParseCommand<'a> for vFileReadlink<'a> { fn from_packet(buf: PacketBuf<'a>) -> Option { - let body = buf.into_body(); + let (buf, body_range) = buf.into_raw_buf(); + let (body, buf) = buf[body_range.start..].split_at_mut(body_range.end - body_range.start); + if body.is_empty() { return None; } @@ -15,7 +19,7 @@ impl<'a> ParseCommand<'a> for vFileReadlink<'a> { match body { [b':', body @ ..] => { let filename = decode_hex_buf(body).ok()?; - Some(vFileReadlink{filename}) + Some(vFileReadlink{filename, buf}) }, _ => None, } diff --git a/src/protocol/response_writer.rs b/src/protocol/response_writer.rs index 64c26d28..13c1a97d 100644 --- a/src/protocol/response_writer.rs +++ b/src/protocol/response_writer.rs @@ -193,25 +193,6 @@ impl<'a, C: Connection + 'a> ResponseWriter<'a, C> { Ok(()) } - /// Write a range of data specified by offset and len. - /// Used by qXfer:_object_:read commands. - pub fn write_binary_range( - &mut self, - data: &[u8], - offset: usize, - len: usize, - ) -> Result<(), Error> { - if offset < data.len() { - // still more data - self.write_str("m")?; - self.write_binary(&data[offset..][..len.min(data.len() - offset)])? - } else { - // no more data - self.write_str("l")?; - } - Ok(()) - } - /// Write a number as a big-endian hex string using the most compact /// representation possible (i.e: trimming leading zeros). pub fn write_num(&mut self, digit: D) -> Result<(), Error> { diff --git a/src/target/ext/host_io.rs b/src/target/ext/host_io.rs index 36a60ee3..52898838 100644 --- a/src/target/ext/host_io.rs +++ b/src/target/ext/host_io.rs @@ -199,30 +199,6 @@ impl From for HostIoError { /// See [`HostIoError`] for more details. pub type HostIoResult = Result::Error>>; -/// Zero-sized type token that ensures HostIoOutput::write is called. -pub struct HostIoToken<'a>(core::marker::PhantomData<&'a *mut ()>); - -/// An interface to send pread data back to the GDB client. -pub struct HostIoOutput<'a> { - cb: &'a mut dyn FnMut(&[u8]), - token: HostIoToken<'a>, -} - -impl<'a> HostIoOutput<'a> { - pub(crate) fn new(cb: &'a mut dyn FnMut(&[u8])) -> Self { - Self { - cb, - token: HostIoToken(core::marker::PhantomData), - } - } - - /// Write out raw file bytes to the GDB debugger. - pub fn write(self, buf: &[u8]) -> HostIoToken<'a> { - (self.cb)(buf); - self.token - } -} - /// Target Extension - Perform I/O operations on host pub trait HostIo: Target { /// Enable open operation. @@ -309,10 +285,10 @@ pub trait HostIoPread: HostIo { fn pread<'a>( &mut self, fd: u32, - count: ::Usize, - offset: ::Usize, - output: HostIoOutput<'a>, - ) -> HostIoResult, Self>; + count: usize, + offset: u64, + buf: &mut [u8], + ) -> HostIoResult; } define_ext!(HostIoPreadOps, HostIoPread); @@ -362,11 +338,7 @@ pub trait HostIoReadlink: HostIo { /// will consume the `output` object and return a [`HostIoToken`]. This /// token ensures that the implementer of this method calls /// [`HostIoOutput::write`]. - fn readlink<'a>( - &mut self, - filename: &[u8], - output: HostIoOutput<'a>, - ) -> HostIoResult, Self>; + fn readlink<'a>(&mut self, filename: &[u8], buf: &mut [u8]) -> HostIoResult; } define_ext!(HostIoReadlinkOps, HostIoReadlink); diff --git a/src/target/ext/memory_map.rs b/src/target/ext/memory_map.rs index 59952f39..79e9a557 100644 --- a/src/target/ext/memory_map.rs +++ b/src/target/ext/memory_map.rs @@ -1,5 +1,5 @@ //! Provide a memory map for the target. -use crate::target::Target; +use crate::target::{Target, TargetResult}; /// Target Extension - Provide a target memory map. pub trait MemoryMap: Target { @@ -8,7 +8,12 @@ pub trait MemoryMap: Target { /// See the [GDB Documentation] for a description of the format. /// /// [GDB Documentation]: https://sourceware.org/gdb/onlinedocs/gdb/Memory-Map-Format.html - fn memory_map_xml(&self) -> &str; + fn memory_map_xml( + &self, + offset: u64, + length: usize, + buf: &mut [u8], + ) -> TargetResult; } define_ext!(MemoryMapOps, MemoryMap); diff --git a/src/target/ext/target_description_xml_override.rs b/src/target/ext/target_description_xml_override.rs index 7cac0340..06068e22 100644 --- a/src/target/ext/target_description_xml_override.rs +++ b/src/target/ext/target_description_xml_override.rs @@ -1,5 +1,5 @@ //! Override the target description XML specified by `Target::Arch`. -use crate::target::Target; +use crate::target::{Target, TargetResult}; /// Target Extension - Override the target description XML specified by /// `Target::Arch`. @@ -13,7 +13,12 @@ pub trait TargetDescriptionXmlOverride: Target { /// Refer to the /// [target_description_xml](crate::arch::Arch::target_description_xml) /// docs for more info. - fn target_description_xml(&self) -> &str; + fn target_description_xml( + &self, + offset: u64, + length: usize, + buf: &mut [u8], + ) -> TargetResult; } define_ext!( From e12debb6acb920e211fa5de0f2acb82ab83e5a7d Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Sat, 18 Sep 2021 11:25:57 +0800 Subject: [PATCH 2/7] Some fixes --- src/gdbstub_impl/ext/base.rs | 25 ++++++++++++------------ src/protocol/commands/_vFile_readlink.rs | 1 + src/target/ext/host_io.rs | 18 ++++++++--------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/gdbstub_impl/ext/base.rs b/src/gdbstub_impl/ext/base.rs index 1f5825a7..dbda7fbd 100644 --- a/src/gdbstub_impl/ext/base.rs +++ b/src/gdbstub_impl/ext/base.rs @@ -126,22 +126,23 @@ impl GdbStubImpl { HandlerStatus::NeedsOk } Base::qXferFeaturesRead(cmd) => { - #[allow(clippy::redundant_closure)] let ret = if let Some(ops) = target.target_description_xml_override() { ops.target_description_xml(cmd.offset, cmd.length, cmd.buf) .handle_error()? + } else if let Some(xml) = T::Arch::target_description_xml() { + let xml = xml.trim().as_bytes(); + let len = xml.len(); + let data = &xml + [len.min(cmd.offset as usize)..len.min(cmd.offset as usize + cmd.length)]; + let buf = &mut cmd.buf[..data.len()]; + buf.copy_from_slice(data); + data.len() } else { - if let Some(xml) = T::Arch::target_description_xml() { - let xml = xml.trim().as_bytes(); - let len = xml.len(); - let data = &xml[len.min(cmd.offset as usize) - ..len.min(cmd.offset as usize + cmd.length)]; - let buf = &mut cmd.buf[..data.len()]; - buf.copy_from_slice(data); - data.len() - } else { - 0 - } + // If the target hasn't provided their own XML, then the initial response to + // "qSupported" wouldn't have included "qXfer:features:read", and gdb + // wouldn't send this packet unless it was + // explicitly marked as supported. + return Err(Error::PacketUnexpected); }; if ret == 0 { diff --git a/src/protocol/commands/_vFile_readlink.rs b/src/protocol/commands/_vFile_readlink.rs index b56201fd..8cc2ef81 100644 --- a/src/protocol/commands/_vFile_readlink.rs +++ b/src/protocol/commands/_vFile_readlink.rs @@ -10,6 +10,7 @@ pub struct vFileReadlink<'a> { impl<'a> ParseCommand<'a> for vFileReadlink<'a> { fn from_packet(buf: PacketBuf<'a>) -> Option { let (buf, body_range) = buf.into_raw_buf(); + // TODO: rewrite to avoid panic let (body, buf) = buf[body_range.start..].split_at_mut(body_range.end - body_range.start); if body.is_empty() { diff --git a/src/target/ext/host_io.rs b/src/target/ext/host_io.rs index 52898838..e105e37f 100644 --- a/src/target/ext/host_io.rs +++ b/src/target/ext/host_io.rs @@ -278,11 +278,12 @@ pub trait HostIoPread: HostIo { /// Up to `count` bytes will be read from the file, starting at `offset` /// relative to the start of the file. /// - /// The data read _must_ be sent by calling [`HostIoOutput::write`], which - /// will consume the `output` object and return a [`HostIoToken`]. This - /// token ensures that the implementer of this method calls - /// [`HostIoOutput::write`]. - fn pread<'a>( + /// Return the number of bytes written into `buf` (which may be less than + /// `count`). + /// + /// If `offset` is greater than the length of the underlying data, return + /// `Ok(0)`. + fn pread( &mut self, fd: u32, count: usize, @@ -334,11 +335,8 @@ define_ext!(HostIoUnlinkOps, HostIoUnlink); pub trait HostIoReadlink: HostIo { /// Read value of symbolic link `filename` on the target. /// - /// The data read _must_ be sent by calling [`HostIoOutput::write`], which - /// will consume the `output` object and return a [`HostIoToken`]. This - /// token ensures that the implementer of this method calls - /// [`HostIoOutput::write`]. - fn readlink<'a>(&mut self, filename: &[u8], buf: &mut [u8]) -> HostIoResult; + /// Return the number of bytes written into `buf`. + fn readlink(&mut self, filename: &[u8], buf: &mut [u8]) -> HostIoResult; } define_ext!(HostIoReadlinkOps, HostIoReadlink); From b9800bb8563a474e11c13c1e22599140cb64f3a4 Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Sat, 18 Sep 2021 15:17:23 +0800 Subject: [PATCH 3/7] Add helper function to copy range of data to buf --- examples/armv4t/gdb/exec_file.rs | 7 ++----- examples/armv4t/gdb/host_io.rs | 21 +++++-------------- examples/armv4t/gdb/memory_map.rs | 8 ++----- examples/armv4t/gdb/mod.rs | 8 +++++++ .../gdb/target_description_xml_override.rs | 7 ++----- 5 files changed, 19 insertions(+), 32 deletions(-) diff --git a/examples/armv4t/gdb/exec_file.rs b/examples/armv4t/gdb/exec_file.rs index c130a9dc..e70a022a 100644 --- a/examples/armv4t/gdb/exec_file.rs +++ b/examples/armv4t/gdb/exec_file.rs @@ -2,6 +2,7 @@ use gdbstub::common::Pid; use gdbstub::target; use gdbstub::target::TargetResult; +use super::copy_range_to_buf; use crate::emu::Emu; impl target::ext::exec_file::ExecFile for Emu { @@ -13,10 +14,6 @@ impl target::ext::exec_file::ExecFile for Emu { buf: &mut [u8], ) -> TargetResult { let filename = b"/test.elf"; - let len = filename.len(); - let data = &filename[len.min(offset as usize)..len.min(offset as usize + length)]; - let buf = &mut buf[..data.len()]; - buf.copy_from_slice(data); - Ok(data.len()) + Ok(copy_range_to_buf(filename, offset, length, buf)) } } diff --git a/examples/armv4t/gdb/host_io.rs b/examples/armv4t/gdb/host_io.rs index 74f2703a..81bd5ff5 100644 --- a/examples/armv4t/gdb/host_io.rs +++ b/examples/armv4t/gdb/host_io.rs @@ -5,6 +5,7 @@ use gdbstub::target::ext::host_io::{ FsKind, HostIoErrno, HostIoError, HostIoOpenFlags, HostIoOpenMode, HostIoResult, HostIoStat, }; +use super::copy_range_to_buf; use crate::emu::Emu; use crate::TEST_PROGRAM_ELF; @@ -138,12 +139,7 @@ impl target::ext::host_io::HostIoPread for Emu { ) -> HostIoResult { if fd < FD_RESERVED { if fd == 0 { - let len = TEST_PROGRAM_ELF.len(); - let data = - &TEST_PROGRAM_ELF[len.min(offset as usize)..len.min(offset as usize + count)]; - let buf = &mut buf[..data.len()]; - buf.copy_from_slice(data); - return Ok(data.len()); + return Ok(copy_range_to_buf(TEST_PROGRAM_ELF, offset, count, buf)); } else { return Err(HostIoError::Errno(HostIoErrno::EBADF)); } @@ -250,15 +246,11 @@ impl target::ext::host_io::HostIoReadlink for Emu { if filename == b"/proc/1/exe" { // Support `info proc exe` command let exe = b"/test.elf"; - let buf = &mut buf[..exe.len()]; - buf.copy_from_slice(exe); - return Ok(exe.len()); + return Ok(copy_range_to_buf(exe, 0, exe.len(), buf)); } else if filename == b"/proc/1/cwd" { // Support `info proc cwd` command let cwd = b"/"; - let buf = &mut buf[..cwd.len()]; - buf.copy_from_slice(cwd); - return Ok(cwd.len()); + return Ok(copy_range_to_buf(cwd, 0, cwd.len(), buf)); } else if filename.starts_with(b"/proc") { return Err(HostIoError::Errno(HostIoErrno::ENOENT)); } @@ -270,10 +262,7 @@ impl target::ext::host_io::HostIoReadlink for Emu { .to_str() .ok_or(HostIoError::Errno(HostIoErrno::ENOENT))? .as_bytes(); - let len = data.len(); - let buf = &mut buf[..len]; - buf.copy_from_slice(data); - Ok(len) + Ok(copy_range_to_buf(data, 0, data.len(), buf)) } } diff --git a/examples/armv4t/gdb/memory_map.rs b/examples/armv4t/gdb/memory_map.rs index 82d26a1d..ddddb654 100644 --- a/examples/armv4t/gdb/memory_map.rs +++ b/examples/armv4t/gdb/memory_map.rs @@ -1,6 +1,7 @@ use gdbstub::target; use gdbstub::target::TargetResult; +use super::copy_range_to_buf; use crate::emu::Emu; impl target::ext::memory_map::MemoryMap for Emu { @@ -21,11 +22,6 @@ impl target::ext::memory_map::MemoryMap for Emu { "# .trim() .as_bytes(); - - let len = memory_map.len(); - let data = &memory_map[len.min(offset as usize)..len.min(offset as usize + length)]; - let buf = &mut buf[..data.len()]; - buf.copy_from_slice(data); - Ok(data.len()) + Ok(copy_range_to_buf(memory_map, offset, length, buf)) } } diff --git a/examples/armv4t/gdb/mod.rs b/examples/armv4t/gdb/mod.rs index 89929cc1..3167b9fd 100644 --- a/examples/armv4t/gdb/mod.rs +++ b/examples/armv4t/gdb/mod.rs @@ -36,6 +36,14 @@ fn cpu_reg_id(id: ArmCoreRegId) -> Option { } } +pub fn copy_range_to_buf(data: &[u8], offset: u64, length: usize, buf: &mut [u8]) -> usize { + let len = data.len(); + let data = &data[len.min(offset as usize)..len.min(offset as usize + length)]; + let buf = &mut buf[..data.len()]; + buf.copy_from_slice(data); + data.len() +} + impl Target for Emu { // As an example, I've defined a custom architecture based off // `gdbstub_arch::arm::Armv4t`. The implementation is in the `custom_arch` diff --git a/examples/armv4t/gdb/target_description_xml_override.rs b/examples/armv4t/gdb/target_description_xml_override.rs index 4fc4d139..714c0716 100644 --- a/examples/armv4t/gdb/target_description_xml_override.rs +++ b/examples/armv4t/gdb/target_description_xml_override.rs @@ -1,6 +1,7 @@ use gdbstub::target; use gdbstub::target::TargetResult; +use super::copy_range_to_buf; use crate::emu::Emu; impl target::ext::target_description_xml_override::TargetDescriptionXmlOverride for Emu { @@ -76,10 +77,6 @@ impl target::ext::target_description_xml_override::TargetDescriptionXmlOverride .trim() .as_bytes(); - let len = xml.len(); - let data = &xml[len.min(offset as usize)..len.min(offset as usize + length)]; - let buf = &mut buf[..data.len()]; - buf.copy_from_slice(data); - Ok(data.len()) + Ok(copy_range_to_buf(xml, offset, length, buf)) } } From 35b8db4578520a4f96e228730649b63fe719d9d8 Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Sun, 19 Sep 2021 10:56:02 +0800 Subject: [PATCH 4/7] Elide slice indexing bounds checks --- examples/armv4t/gdb/mod.rs | 4 ++++ src/gdbstub_impl/ext/base.rs | 24 +++++++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/examples/armv4t/gdb/mod.rs b/examples/armv4t/gdb/mod.rs index 3167b9fd..7f03cab1 100644 --- a/examples/armv4t/gdb/mod.rs +++ b/examples/armv4t/gdb/mod.rs @@ -36,6 +36,10 @@ fn cpu_reg_id(id: ArmCoreRegId) -> Option { } } +/// Copy a range of `data` (start at `offset` with a size of `length`) to `buf`. +/// Return the size of data copied. Return 0 if encounter EOF. +/// +/// Mainly used by qXfer:_object_:read commands. pub fn copy_range_to_buf(data: &[u8], offset: u64, length: usize, buf: &mut [u8]) -> usize { let len = data.len(); let data = &data[len.min(offset as usize)..len.min(offset as usize + length)]; diff --git a/src/gdbstub_impl/ext/base.rs b/src/gdbstub_impl/ext/base.rs index dbda7fbd..561cd106 100644 --- a/src/gdbstub_impl/ext/base.rs +++ b/src/gdbstub_impl/ext/base.rs @@ -131,17 +131,23 @@ impl GdbStubImpl { .handle_error()? } else if let Some(xml) = T::Arch::target_description_xml() { let xml = xml.trim().as_bytes(); - let len = xml.len(); - let data = &xml - [len.min(cmd.offset as usize)..len.min(cmd.offset as usize + cmd.length)]; - let buf = &mut cmd.buf[..data.len()]; - buf.copy_from_slice(data); - data.len() + let xml_len = xml.len(); + + let start = xml_len.min(cmd.offset as usize); + let end = xml_len.min(cmd.offset as usize + cmd.length); + + // LLVM isn't smart enough to realize that `end` will always be greater than + // `start`, and fails to elide the `slice_index_order_fail` check unless we + // include this seemingly useless call to `max`. + let data = &xml[start..end.max(start)]; + + let n = data.len().min(cmd.buf.len()); + cmd.buf[..n].copy_from_slice(&data[..n]); + n } else { // If the target hasn't provided their own XML, then the initial response to - // "qSupported" wouldn't have included "qXfer:features:read", and gdb - // wouldn't send this packet unless it was - // explicitly marked as supported. + // "qSupported" wouldn't have included "qXfer:features:read", and gdb wouldn't + // send this packet unless it was explicitly marked as supported. return Err(Error::PacketUnexpected); }; From 8e640c4ad5cb5febb0026f17aaf03c13773c8f47 Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Mon, 20 Sep 2021 10:24:40 +0800 Subject: [PATCH 5/7] Update src/target/ext/host_io.rs Co-authored-by: Daniel Prilik --- src/target/ext/host_io.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/target/ext/host_io.rs b/src/target/ext/host_io.rs index e105e37f..9161ff8f 100644 --- a/src/target/ext/host_io.rs +++ b/src/target/ext/host_io.rs @@ -336,6 +336,10 @@ pub trait HostIoReadlink: HostIo { /// Read value of symbolic link `filename` on the target. /// /// Return the number of bytes written into `buf`. + /// + /// Unlike most other Host IO handlers, if the resolved file path exceeds + /// the length of the provided `buf`, the target should NOT return a + /// partial response, and MUST return a `Err(HostIoErrno::ENAMETOOLONG)`. fn readlink(&mut self, filename: &[u8], buf: &mut [u8]) -> HostIoResult; } From 532c33328407c0412f974e9f6c5f405e16914383 Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Tue, 21 Sep 2021 09:34:10 +0800 Subject: [PATCH 6/7] Add more checks on copy_range_to_buf --- examples/armv4t/gdb/host_io.rs | 6 +++++- examples/armv4t/gdb/mod.rs | 10 +++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/examples/armv4t/gdb/host_io.rs b/examples/armv4t/gdb/host_io.rs index 81bd5ff5..80a9771f 100644 --- a/examples/armv4t/gdb/host_io.rs +++ b/examples/armv4t/gdb/host_io.rs @@ -262,7 +262,11 @@ impl target::ext::host_io::HostIoReadlink for Emu { .to_str() .ok_or(HostIoError::Errno(HostIoErrno::ENOENT))? .as_bytes(); - Ok(copy_range_to_buf(data, 0, data.len(), buf)) + if data.len() <= buf.len() { + Ok(copy_range_to_buf(data, 0, data.len(), buf)) + } else { + Err(HostIoError::Errno(HostIoErrno::ENAMETOOLONG)) + } } } diff --git a/examples/armv4t/gdb/mod.rs b/examples/armv4t/gdb/mod.rs index 7f03cab1..3eb22095 100644 --- a/examples/armv4t/gdb/mod.rs +++ b/examples/armv4t/gdb/mod.rs @@ -1,4 +1,4 @@ -use core::convert::TryInto; +use core::convert::{TryFrom, TryInto}; use armv4t_emu::{reg, Memory}; use gdbstub::target; @@ -37,12 +37,16 @@ fn cpu_reg_id(id: ArmCoreRegId) -> Option { } /// Copy a range of `data` (start at `offset` with a size of `length`) to `buf`. -/// Return the size of data copied. Return 0 if encounter EOF. +/// Return the size of data copied. Returns 0 if `offset >= buf.len()`. /// /// Mainly used by qXfer:_object_:read commands. pub fn copy_range_to_buf(data: &[u8], offset: u64, length: usize, buf: &mut [u8]) -> usize { + let offset = match usize::try_from(offset) { + Ok(v) => v, + Err(_) => return 0, + }; let len = data.len(); - let data = &data[len.min(offset as usize)..len.min(offset as usize + length)]; + let data = &data[len.min(offset)..len.min(offset + length)]; let buf = &mut buf[..data.len()]; buf.copy_from_slice(data); data.len() From b3e31e1a6db04ba7f1bf2aa70075970952433843 Mon Sep 17 00:00:00 2001 From: Bet4 <0xbet4@gmail.com> Date: Wed, 22 Sep 2021 08:38:03 +0800 Subject: [PATCH 7/7] Final fix --- examples/armv4t/gdb/target_description_xml_override.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/examples/armv4t/gdb/target_description_xml_override.rs b/examples/armv4t/gdb/target_description_xml_override.rs index 714c0716..a0c7563b 100644 --- a/examples/armv4t/gdb/target_description_xml_override.rs +++ b/examples/armv4t/gdb/target_description_xml_override.rs @@ -76,7 +76,6 @@ impl target::ext::target_description_xml_override::TargetDescriptionXmlOverride "# .trim() .as_bytes(); - Ok(copy_range_to_buf(xml, offset, length, buf)) } }