Messages in this thread |  | | From | Wedson Almeida Filho <> | Date | Sun, 4 Dec 2022 17:26:54 +0000 | Subject | Re: [PATCH v2 20/28] rust: str: add `Formatter` type |
| |
On Sun, 4 Dec 2022 at 15:41, Dr. David Alan Gilbert <dave@treblig.org> wrote: > > * ojeda@kernel.org (ojeda@kernel.org) wrote: > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > Add the `Formatter` type, which leverages `RawFormatter`, > > but fails if callers attempt to write more than will fit > > in the buffer. > > > > In order to so, implement the `RawFormatter::from_buffer()` > > constructor as well. > > > > Co-developed-by: Adam Bratschi-Kaye <ark.email@gmail.com> > > Signed-off-by: Adam Bratschi-Kaye <ark.email@gmail.com> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > Reviewed-by: Gary Guo <gary@garyguo.net> > > [Reworded, adapted for upstream and applied latest changes] > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > > --- > > rust/kernel/str.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > > index a995db36486f..ce207d1b3d2a 100644 > > --- a/rust/kernel/str.rs > > +++ b/rust/kernel/str.rs > > @@ -406,6 +406,23 @@ impl RawFormatter { > > } > > } > > > > + /// Creates a new instance of [`RawFormatter`] with the given buffer. > > + /// > > + /// # Safety > > + /// > > + /// The memory region starting at `buf` and extending for `len` bytes must be valid for writes > > + /// for the lifetime of the returned [`RawFormatter`]. > > + pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self { > > + let pos = buf as usize; > > + // INVARIANT: We ensure that `end` is never less then `buf`, and the safety requirements > > + // guarantees that the memory region is valid for writes. > > + Self { > > + pos, > > + beg: pos, > > + end: pos.saturating_add(len), > > + } > > + } > > + > > /// Returns the current insert position. > > /// > > /// N.B. It may point to invalid memory. > > @@ -439,3 +456,43 @@ impl fmt::Write for RawFormatter { > > Ok(()) > > } > > } > > + > > +/// Allows formatting of [`fmt::Arguments`] into a raw buffer. > > +/// > > +/// Fails if callers attempt to write more than will fit in the buffer. > > +pub(crate) struct Formatter(RawFormatter);
Here we mention that `Formatter` fails if callers attempt to write more than will fit in the buffer.
This is in contrast with `RawFormatter`, which doesn't fail in such cases. There's also a comment there explaining it (not visible in this patch because it's already there), but I reproduce below:
/// Allows formatting of [`fmt::Arguments`] into a raw buffer. /// /// It does not fail if callers write past the end of the buffer so that they can calculate the /// size required to fit everything. /// /// # Invariants /// /// The memory region between `pos` (inclusive) and `end` (exclusive) is valid for writes if `pos` /// is less than `end`. pub(crate) struct RawFormatter {
`RawFormatter` is used to implement the "%pA" printf specifier, which requires this behaviour.
> > + > > +impl Formatter { > > + /// Creates a new instance of [`Formatter`] with the given buffer. > > + /// > > + /// # Safety > > + /// > > + /// The memory region starting at `buf` and extending for `len` bytes must be valid for writes > > + /// for the lifetime of the returned [`Formatter`]. > > + #[allow(dead_code)] > > + pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self { > > + // SAFETY: The safety requirements of this function satisfy those of the callee. > > + Self(unsafe { RawFormatter::from_buffer(buf, len) }) > > + } > > +} > > + > > +impl Deref for Formatter { > > + type Target = RawFormatter; > > + > > + fn deref(&self) -> &Self::Target { > > + &self.0 > > + } > > +} > > + > > +impl fmt::Write for Formatter { > > + fn write_str(&mut self, s: &str) -> fmt::Result { > > + self.0.write_str(s)?; > > + > > + // Fail the request if we go past the end of the buffer. > > Reading this for the first time, I'm surprised by this, perhaps a > bit more comment is needed? I was expecting that nothing would > let pos pass end.
Given the comments I highlight above, do you think we still need more?
(My impression is that you're reading this without the context I tried to explain above, and this context may perhaps be sufficient.)
Thanks, -Wedson
|  |