From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 6D9C21FF0E1 for ; Mon, 10 Aug 2026 14:39:49 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3E37B2176B; Mon, 10 Aug 2026 14:39:49 +0200 (CEST) Message-ID: <36e1aa51-9a25-4b49-a6d1-5675286117d9@proxmox.com> Date: Mon, 10 Aug 2026 14:39:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation To: "Max R. Carrara" References: <20260805141620.4190773-1-m.carrara@proxmox.com> <20260805141620.4190773-2-m.carrara@proxmox.com> <178601383145.99099.751352518821996428.b4-review@b4> <590f88b8-e034-446d-a307-8505e2ca787d@proxmox.com> Content-Language: en-US, de-AT From: Robert Obkircher In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786365530068 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.873 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: FB6Y4JM2SZVC7CX43ZRK7ZTDOQAXD52X X-Message-ID-Hash: FB6Y4JM2SZVC7CX43ZRK7ZTDOQAXD52X X-MailFrom: r.obkircher@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 10.08.26 11:51, Max R. Carrara wrote: > [..] > > Could you perhaps show what would make it easier to reason about for you > instead? Maybe I'm just missing something here. The indirection just makes it easier to overlook things. For example, if someone increased the size of PayloadType it is now very easy to overlook that it could result in an allocation that is bigger that size_of_val(t). Not an actual problem, though. >>>>> + >>>>> [..] >>>>> >>>>> +// Putting underlying implementation in here to prevent access to private fields >>>>> >>>> This seems like a confusing, unrelated change with no benefit. >>> The main benefit here is that private fields cannot be accessed anymore. >> I'm not fully convinced that the definition of an extern format needs >> to be hidden from someone willing to work at that level. > What do you mean with hidden? All fields are still accessible through > their respective methods, it's just that *directly* modifying those > fields is not as easy as before anymore. There wasn't any point in > exposing the fields as `pub` in the first place IMO, and I also think > that doing that encourages bad patterns ... TLDR, since we already discussed this offline: I think fields are slightly better for this specific case because it's plain old data and you can mutably borrow more than one at a time. Not that important, though. > > For example, in both the [reader] and [writer] implementations we are > casting the entire struct to a byte slice directly, even though that > could've been a method each instead, keeping the implementation of such > an operation local to the struct. That's why I introduced `as_bytes()` > and `as_bytes_mut()`, which do exactly that. Those two are great! By accessors I meant getters and setters. We should seriously consider making zerocopy an explicit dependency. It is already used by rand and hashbrown and it could handle this exact case [1] and the byte-order aware U24 length field [2] without any unsafe code on our part. I was already considering it for the index readers and Fabian said he wouldn't mind if we could eventually get rid of endian_trait(_derive) that way. [1] https://docs.rs/zerocopy/latest/zerocopy/byteorder/index.html#example [2] https://docs.rs/u24/latest/u24/unaligned/index.html > [..] > >> There is a [2]nd one that I had posted on Zulip, and the third could >> be a unit test that checks an Rc count. >> >> I just prefer mechanical changes like moving code to be in separate >> commits. >> >> [2] >> https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-tape/src/sgutils2.rs;h=340616c73675b994c8761a6981fd41e2e249f953;hb=HEAD#l447 > Hmm, fair point! I'll see if I can fix the second instance in v2 too, > then. > > Which third one are you referring to exactly? I just meant a unit test that can be run without Zmiri-disable-isolation to verify that drop is invoked. > [..]