Re: [PATCH 16/20] rust: pin-init: internal: pin_data: make field lifetime invariance imply type invariance

["Gary Guo" <gary@garyguo.net>]

(Cc list)

TLDR: false positive. Explanations below.

On Thu Oct 8, 2026 at 1:40 PM BST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] rust: pin_data macro drop order check bypassed for covariant fields
> --
>
> commit f9d502571fb94f74de22024871d23b994623a8b0
> Author: Gary Guo <gary@garyguo.net>
>
> rust: pin-init: internal: pin_data: make field lifetime invariance imply type invariance
>
> The commit modifies pin_data to ensure invariant capturing of fields.
> It splits the outlive chain into covariant and invariant fields to avoid
> outlive relationships issues. It calculates an invariant closure and
> handles type invariance.
>
>> diff --git a/rust/pin-init/internal/src/pin_data.rs b/rust/pin-init/internal/src/pin_data.rs
>> index d42d86ffd8538..b35d32ff2bb2d 100644
>> --- a/rust/pin-init/internal/src/pin_data.rs
>> +++ b/rust/pin-init/internal/src/pin_data.rs
>
> [ ... ]
>
>> @@ -520,6 +581,96 @@ fn expand(
>>          }
>>      }
>>  
>> +    let mut field_lts_split_variance_outlive_chain = Generics {
>> +        lt_token: Some(Default::default()),
>> +        params: borrowed_covariant_fields
>
> [Severity: Critical]
> Does splitting the chains by variance in expand() erase the relative
> declaration-order bounds between covariant and invariant fields?
>
> The drop-order check relies on the outlive chains, and without
> declaration-order bounds, the compiler might not be able to safely enforce
> drop ordering.

Omitting a bound can never cause soundness issue (at most they will cause
something that is safe fail to compile).

Here, these bounds are deliberately split, so we cannot arrive with a `Param:
'invariant_field` bound transitively.

>
>> +            .iter()
>> +            .zip(std::iter::once(None).chain(borrowed_covariant_fields.iter().map(Some)))
>
> [ ... ]
>
>> +        for capture in field.captures.iter() {
>> +            let Some(&idx) = field_idx_map.get(&capture.lifetime.ident) else {
>> +                continue;
>> +            };
>> +
>> +            let prev_borrowed = fields[idx].borrowed.as_ref().unwrap();
>> +
>> +            // If borrowed field is covariant, it's already captured in
>> +            // `field_lts_split_variance_outlive_chain`.
>> +            if prev_borrowed.lt_variance == Variance::Invariant {
>> +                // Covariant field borrowing an invariant field. This is not captured in the chain
>> +                // so we need to add additional bound. This bound is okay, as the invariant lifetime
>> +                // is the longer living one, so arbitrary shortening of the covariant one does not
>> +                // violate their relation.
>> +                let param = field_lts_split_variance_outlive_chain
>> +                    .lifetimes_mut()
>> +                    .find(|l| l.lifetime == prev_borrowed.lifetime)
>> +                    .unwrap();
>> +                param.bounds.push(borrowed.lifetime.clone());
>
> [Severity: Critical]
> By injecting this bound unconditionally in expand(), is the declaration order
> check bypassed?
>
> This could effectively blind the compiler to drop-order inversions between
> these fields, potentially allowing fields to access freed memory during Drop.

The field-lifetime-outliving-captured-lifetime check is a custom check in
drop_order_check and not bypassed. We do not rely on compiler to capture this.

Best,
Gary

>
>> +            }
>> +        }
>> +    }