all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "Max R. Carrara" <m.carrara@proxmox.com>
To: "Robert Obkircher" <r.obkircher@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Subject: Re: [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type
Date: Mon, 07 Sep 2026 15:40:13 +0200	[thread overview]
Message-ID: <DL94KKP88LQC.68WVOW3HT864@proxmox.com> (raw)
In-Reply-To: <4943f2e3-8bba-4388-b2c7-f7f714044208@proxmox.com>

On Mon Sep 7, 2026 at 2:50 PM CEST, Robert Obkircher wrote:
>
> On 07.09.26 13:44, Max R. Carrara wrote:
> >>> [..]
> >>>
> >>> +
> >>> +    /// Create a new [`LayoutAwareBox<T>`] from a pointer and a [`Layout`].
> >>> +    ///
> >>> +    /// Note that if `T` is [zero-sized], no allocation is actually performed.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// Improper use of this function can lead to [undefined behavior] and other
> >>> +    /// issues.
> >>> +    ///
> >>> +    /// In general, the same safety requirements as for [`Box::from_raw`] apply
> >>> +    /// for this function.
> >>> +    ///
> >>> +    /// **Additionally,** the caller must also guarantee that the passed pointer
> >>> +    /// points to a `T` which was allocated using the passed layout **and** has
> >>> +    /// the same size as its underlying allocation.
> >>> +    ///
> >>> +    /// The latter in particular is easy to miss: If you allocate `1024` bytes
> >>> +    /// for a struct that only takes up `1000` bytes for some reason,
> >>> +    /// deallocating this struct will *still* be considered undefined behavior
> >>> +    /// and will be spotted by [Miri].
> >> That reason might be pointer provenance. Once you shrink down the
> >> spacial memory range a pointer is allowed to access, you are not
> >> allowed to extend it back to the full size (see std::ptr module docs).
> >>
> >> Makes me wonder if this is even safe:
> >> ptr::slice_from_raw_parts_mut(thin_ptr, len) as *mut DST;
> >>
> >> I'll have to read up on this.
> > Hm, why would that call not be safe? Assuming the size / len
> > calculations are all correct etc.
> The call itself is definitely safe, the question is whether the result
> can be dereferenced.
> >
> > All that `ptr::slice_from_raw_parts_mut` does is say "there are `len`
> > elements in the (trailing) slice of `T` that `thin_ptr` points to" --
> > I'm not sure it affects the provenance of the pointer in this case..?
> >
> > Actually, reading up on [provenance] now, there's a paragraph that
> > mentions the following:
> >
> >> The Original Pointer for an allocation has provenance that constrains
> >> the spatial permissions of this pointer to the memory range of the
> >> allocation, and the temporal permissions to the lifetime of the
> >> allocation. Provenance is implicitly inherited by all pointers
> >> transitively derived from the Original Pointer through operations like
> >> offset, borrowing, and pointer casts.
> > The last sentence here I think is key, since *I think* that in this
> > context, `ptr::slice_from_raw_parts_mut` should be considered a pointer
> > cast, since it's a thin-to-fat conversion, only changing the pointer's
> > metadata. I therefore would say that this does not create new
> > provenance, since it only constrains the slice's valid range.
> I also think so.
> >
> > Then again, what pointer provenance is is not concretely defined yet
> > anyways, so I'm not sure if it's worth worrying over that...
> >
> > (Also, that pattern for DSTs is well-established in the ecosystem, tbf.)
> >
> > [provenance] https://doc.rust-lang.org/std/ptr/index.html#provenance
> >
> >>
> >> [..]
> >> +
> >> +        for i in 0..len {
> >> +            // SAFETY: thin_ptr is valid for writing and we never go out of
> >> +            // bounds during iteration.
> >> +            unsafe { thin_ptr.add(i).write(value.clone()) };
> >> +        }
> >>
> >> nit: this doesn't drop the previous elements if clone panics [...]
> > Oh, that's true actually. Thanks for pointing this out, will see how
> > I'll fix this in v2.
> To be clear: It is technically safe not to call drop.
> >
> >> [...] and there is an unnecessary copy in the last iteration.
> > What exactly do you mean?
> You call clone() n times instead of n-1. Not the end of the world, though.
>
> https://github.com/rust-lang/rust/blob/656a9da186dacaf3bf8f7f7296a825d256cb4ae3/library/alloc/src/vec/mod.rs#L3759

Oh, right! Damn, thanks for spotting this.

>
> >
> >> Couldn't this entire function just forward to slice_fill_with? The only
> >> insteresting special case is if `value` was all zeroes.
> > Hmm, I think it could, but I'll have to double-check.
> Unfortunately, the IsZero trait is not available outside of the alloc
> crate:
>
> https://github.com/rust-lang/rust/blob/656a9da186dacaf3bf8f7f7296a825d256cb4ae3/library/alloc/src/vec/spec_from_elem.rs#L24

Ah, thanks for the link!

>
> > [..]





  reply	other threads:[~2026-09-07 13:40 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
2026-09-02 16:14   ` Robert Obkircher
2026-09-07 11:44     ` Max R. Carrara
2026-09-07 12:50       ` Robert Obkircher
2026-09-07 13:40         ` Max R. Carrara [this message]
2026-08-21 14:02 ` [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs Max R. Carrara
2026-09-02 16:14   ` Robert Obkircher
2026-09-07 11:44     ` Max R. Carrara
2026-09-02 16:17   ` Robert Obkircher
2026-09-07 11:44     ` Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 03/10] tape: move tape block structs into separate file module Max R. Carrara
2026-09-02 16:17   ` Robert Obkircher
2026-09-07 11:44     ` Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 04/10] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 05/10] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 06/10] tape: tape block: represent tape block header with its own struct Max R. Carrara
2026-09-02 16:17   ` Robert Obkircher
2026-09-07 11:44     ` Max R. Carrara
2026-09-07 13:03       ` Robert Obkircher
2026-08-21 14:02 ` [PATCH proxmox-backup v2 07/10] tape: tape block: make `payload` field private Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 08/10] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara
2026-09-02 16:36 ` [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Robert Obkircher
2026-09-07 11:44   ` Max R. Carrara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DL94KKP88LQC.68WVOW3HT864@proxmox.com \
    --to=m.carrara@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=r.obkircher@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal